Repository navigation
OCPBUGS-83757: Remove network dependencies from unit tests - #8277
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@sdminonne: This pull request references Jira Issue OCPBUGS-83757, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughIntroduces injectable hooks for image-metadata and repo-setup verification: RegistryClientImageMetadataProvider gains an injectable metadataGetter (with getMetadataGetter) and a provider method seekOverride that uses provider.OpenShiftImageRegistryOverrides and the injected getter. ProviderWithOpenShiftImageRegistryOverridesDecorator gains a repoSetupFn field and uses it (falling back to registryclient.GetRepoSetup) during mirror verification. Fake image-metadata provider adds an Err field, returns fake manifests directly, and parses image references in GetDigest. Tests were refactored to inject mocks, inline pull secrets, and avoid filesystem/network access. Sequence Diagram(s)sequenceDiagram
participant Consumer as Caller
participant Provider as RegistryClientImageMetadataProvider
participant Getter as metadataGetter (injected)
participant RepoSetup as repoSetupFn / registryclient.GetRepoSetup
participant Registry as Remote Registry
Consumer->>Provider: seekOverride(ctx, parsedImageRef, pullSecret)
Provider->>Getter: getMetadata(ctx, candidateMirrorRef, pullSecret)
alt Getter returns metadata (mirror reachable)
Getter-->>Provider: metadata (success)
Provider->>Consumer: return candidateMirrorRef
else Getter returns error (mirror unreachable)
Getter-->>Provider: error
Provider->>RepoSetup: repoSetup(ctx, candidateMirrorRef, pullSecret)
alt repoSetup returns repository (repo accessible)
RepoSetup-->>Provider: repository, parsedRef, nil
Provider->>Consumer: return parsedRef
else repoSetup returns error
RepoSetup-->>Provider: nil, nil, error
Provider->>Consumer: return original parsedImageRef
end
end
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/control-plane-operator
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
support/util/imagemetadata_test.go (1)
1026-1032:TestSeekOverrideTimeoutno longer exercises timeout-specific behavior.The current failing getter returns immediately, so this test now checks generic failure fallback rather than timeout handling. Consider renaming it or making the fake block on
ctx.Done()to preserve timeout semantics.Possible deterministic timeout-focused test tweak
-func TestSeekOverrideTimeout(t *testing.T) { +func TestSeekOverrideTimeout(t *testing.T) { @@ - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) + defer cancel() @@ - failingMetadataGetter := func(ctx context.Context, imageRef string, pullSecret []byte) (*dockerv1client.DockerImageConfig, []distribution.Descriptor, distribution.BlobStore, error) { - return nil, nil, nil, fmt.Errorf("simulated mirror unavailable") - } + failingMetadataGetter := func(ctx context.Context, imageRef string, pullSecret []byte) (*dockerv1client.DockerImageConfig, []distribution.Descriptor, distribution.BlobStore, error) { + <-ctx.Done() + return nil, nil, nil, ctx.Err() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@support/util/imagemetadata_test.go` around lines 1026 - 1032, The test TestSeekOverrideTimeout no longer simulates a timeout because failingMetadataGetter returns immediately; update the test so it either (A) is renamed to reflect generic failure behavior, or (B) preserves timeout semantics by changing failingMetadataGetter to block until ctx.Done() (e.g., wait for ctx cancellation and then return a context error) so SeekOverride is exercised under a real timeout using the existing ctx and parsedRef/overrides; adjust assertions accordingly to expect timeout-driven fallback when using the blocking getter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@support/util/imagemetadata_test.go`:
- Around line 1026-1032: The test TestSeekOverrideTimeout no longer simulates a
timeout because failingMetadataGetter returns immediately; update the test so it
either (A) is renamed to reflect generic failure behavior, or (B) preserves
timeout semantics by changing failingMetadataGetter to block until ctx.Done()
(e.g., wait for ctx cancellation and then return a context error) so
SeekOverride is exercised under a real timeout using the existing ctx and
parsedRef/overrides; adjust assertions accordingly to expect timeout-driven
fallback when using the blocking getter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c3c731c-8940-48b3-a4b0-176ffa2499cd
📒 Files selected for processing (3)
support/releaseinfo/registryclient/client_test.gosupport/util/imagemetadata.gosupport/util/imagemetadata_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8277 +/- ##
==========================================
+ Coverage 36.42% 36.87% +0.45%
==========================================
Files 765 747 -18
Lines 93302 91669 -1633
==========================================
- Hits 33981 33799 -182
+ Misses 56606 55190 -1416
+ Partials 2715 2680 -35
... and 22 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
support/util/util_test.go (1)
726-728: Returnnilmanifest whenerris set in the fake.Line 727 currently returns both a manifest and an error. Returning
nil, errgives more realistic behavior and prevents accidental use of stale values in failing paths.Proposed fix
func (f *fakeImageMetadataProviderForTest) GetManifest(_ context.Context, _ string, _ []byte) (distribution.Manifest, error) { - return &fakeManifestForTest{mediaType: f.mediaType}, f.err + if f.err != nil { + return nil, f.err + } + return &fakeManifestForTest{mediaType: f.mediaType}, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@support/util/util_test.go` around lines 726 - 728, In fakeImageMetadataProviderForTest.GetManifest change the return behavior so that when f.err is set you return (nil, f.err) instead of returning a fakeManifestForTest plus the error; update the function (GetManifest) to check f.err and return nil,f.err on error and only return &fakeManifestForTest{mediaType: f.mediaType}, nil when no error is present to avoid leaking a stale manifest on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert_test.go`:
- Around line 28-35: The test seeds package-global caches (openIDURLsCache.Set
and oidcPasswordCheckCache.Set) but does not restore them; modify the test to
call t.Cleanup to remove or restore those entries after seeding so state doesn't
leak across tests—e.g., after calling openIDURLsCache.Set and
oidcPasswordCheckCache.Set, register cleanup functions that delete the specific
keys from openIDURLsCache and oidcPasswordCheckCache (or restore previous
values) so the global caches are reset when the test finishes.
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert_test.go`:
- Around line 28-35: The test is mutating package-level caches openIDURLsCache
and oidcPasswordCheckCache directly which risks cross-test leakage; wrap these
cache population steps so you save prior state (e.g., previous entries or
existence flags) and register a t.Cleanup that restores or deletes the inserted
keys (the "https://accounts.google.com/.well-known/openid-configuration" entry
and the "1" key) or fully resets the caches after the test, and ensure you use
the same TTL variables openIDURLsTTL and oidcPasswordTTL when restoring so the
test leaves openIDURLsCache and oidcPasswordCheckCache unchanged for other
tests.
In `@support/releaseinfo/registry_image_content_policies_test.go`:
- Around line 42-44: The injected repoSetupFn currently ignores errors from
reference.Parse which can let invalid image refs pass in tests; modify
repoSetupFn so it captures the error returned by reference.Parse(imageRef) and
return that error (instead of nil) when parse fails, e.g., change the code path
in repoSetupFn that calls reference.Parse to check the error and propagate it
back to the caller so tests fail on invalid refs.
In `@support/util/util_test.go`:
- Around line 730-733: The fakeImageMetadataProviderForTest methods currently
ignore errors from reference.Parse (e.g., in GetDigest) which masks invalid
image refs; update these methods to capture the parse error (ref, err :=
reference.Parse(imageRef)), and if err != nil return an appropriate zero values
plus the parse error instead of discarding it; apply the same change to the
other fake provider method(s) around lines 739-742 so all parse failures are
propagated to callers.
---
Nitpick comments:
In `@support/util/util_test.go`:
- Around line 726-728: In fakeImageMetadataProviderForTest.GetManifest change
the return behavior so that when f.err is set you return (nil, f.err) instead of
returning a fakeManifestForTest plus the error; update the function
(GetManifest) to check f.err and return nil,f.err on error and only return
&fakeManifestForTest{mediaType: f.mediaType}, nil when no error is present to
avoid leaking a stale manifest on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f4d8096-3389-430a-8310-86494e189136
📒 Files selected for processing (7)
control-plane-operator/controllers/hostedcontrolplane/oauth/idp_convert_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/oauth/idp_convert_test.gosupport/catalogs/images_test.gosupport/releaseinfo/registry_image_content_policies.gosupport/releaseinfo/registry_image_content_policies_test.gosupport/util/fakeimagemetadataprovider/fakeimagemetadataprovider.gosupport/util/util_test.go
| expectedExists: false, | ||
| expectedError: true, | ||
| pullSecret: []byte("12345"), | ||
| name: "Management cluster should fail when image not found", |
There was a problem hiding this comment.
Could you please change the test names to follow Gherkin format "When...it should format"?
| } | ||
|
|
||
| // fakeImageMetadataProviderForTest is a minimal ImageMetadataProvider for unit tests. | ||
| type fakeImageMetadataProviderForTest struct { |
There was a problem hiding this comment.
Why not just extend the shared FakeRegistryClientImageMetadataProvider adding a Err field?
| } | ||
|
|
||
| func SeekOverride(ctx context.Context, openshiftImageRegistryOverrides map[string][]string, parsedImageReference reference.DockerImageReference, pullSecret []byte) *reference.DockerImageReference { | ||
| func SeekOverride(ctx context.Context, openshiftImageRegistryOverrides map[string][]string, parsedImageReference reference.DockerImageReference, pullSecret []byte, metadataGetter MetadataGetter) *reference.DockerImageReference { |
There was a problem hiding this comment.
Refactor SeekOverride to use struct field injection instead of a public function parameter. Make MetadataGetter a field on RegistryClientImageMetadataProvider, consistent with the repoSetupFn pattern already used in this PR.
| }, | ||
| // Mock repoSetupFn to avoid real network calls for mirror verification. | ||
| repoSetupFn: func(ctx context.Context, imageRef string, pullSecret []byte) (distribution.Repository, *reference.DockerImageReference, error) { | ||
| ref, _ := reference.Parse(imageRef) |
There was a problem hiding this comment.
Check errors instead of discard.
There was a problem hiding this comment.
Rename it to something that reflects the current behavior. It no longer tests timeout.
| } | ||
|
|
||
| // MetadataGetter is a function that retrieves image metadata from a registry. | ||
| type MetadataGetter func(ctx context.Context, imageRef string, pullSecret []byte) (*dockerv1client.DockerImageConfig, []distribution.Descriptor, distribution.BlobStore, error) |
There was a problem hiding this comment.
Why this needs to be public?
|
@jparrill ptal |
|
/verified by unit-tests |
openshift-ci-robot
added
the
verified
|
@sdminonne: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@coderabbitai ptal |
|
✅ Actions performedReview triggered.
|
|
/approve |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@jparrill: This pull request references Jira Issue OCPBUGS-83757, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill, sdminonne The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/lgtm |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
OCPBUGS-83757 SeekOverride() called getMetadata() directly to verify mirror availability, making real HTTP calls to container registries. This caused test failures when the network was unavailable or slow in CI. Add a MetadataGetter parameter to SeekOverride so tests can inject a fake. When nil, the real getMetadata is used, keeping production behavior unchanged. Also remove GetRepoSetup calls from the registryclient test fake provider, replacing them with local reference.Parse() calls that don't require network access. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
OCPBUGS-83757 Remove GetRepoSetup calls from the shared FakeRegistryClientImageMetadataProvider used across ~20 test files. This was the root cause for widespread offline test failures beyond the two files fixed in the previous commit. Also replace direct use of RegistryClientImageMetadataProvider in util_test.go with a local fake, make the catalogs "image not found" test explicit about its error expectation, add injectable repoSetupFn to ProviderWithOpenShiftImageRegistryOverridesDecorator, and pre-populate the OpenID discovery cache in oauth tests to avoid HTTP calls to accounts.google.com. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Refactor SeekOverride into unexported seekOverride method on RegistryClientImageMetadataProvider with metadataGetter struct field injection, consistent with the repoSetupFn pattern - Remove public MetadataGetter type in favor of unexported metadataGetterFn - Extend shared FakeRegistryClientImageMetadataProvider with Err field and remove duplicate local fakeImageMetadataProviderForTest - Fix FakeManifest.Payload() to return valid JSON - Add t.Cleanup for global cache mutations in oauth idp_convert tests to prevent cross-test state leakage - Propagate reference.Parse errors in test fakes instead of discarding - Rename TestSeekOverrideTimeout to TestSeekOverrideFallsBackWhenMirrorUnavailable - Rename test cases in images_test.go to Gherkin format Jira: https://issues.redhat.com/browse/OCPBUGS-83757 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
44ce39e to
cc16278
Compare
openshift-ci-robot
removed
the
verified
Exclude test fake packages from codecov analysis since they are test helpers analogous to already-excluded mock files. Add tests for the nil-fallback paths in getMetadataGetter and repoSetupFn to cover the default injection logic without requiring network calls. Jira: OCPBUGS-83757 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
cc16278 to
5f640b4
Compare
|
/verified by unit-test |
openshift-ci-robot
added
the
verified
|
@sdminonne: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
Scheduling tests matching the |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
/hold Revision 5f640b4 was retested 3 times: holding |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Now I have the complete picture. Here is the analysis: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe
On 2026-04-30, two commits landed on
The PR branch Recommendations
Evidence
|
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
@sdminonne: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
@sdminonne: Jira Issue Verification Checks: Jira Issue OCPBUGS-83757 Jira Issue OCPBUGS-83757 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Fix included in release 5.0.0-0.nightly-2026-05-02-042818 |
6 participants
Summary
make testpasses 100% with no networkMetadataGetterparameter toSeekOverrideso tests can avoid real HTTP calls to container registriesGetRepoSetupcalls fromregistryclienttest fake and sharedFakeRegistryClientImageMetadataProvider(affects ~20 test files)repoSetupFntoProviderWithOpenShiftImageRegistryOverridesDecoratorfor mirror verificationRegistryClientImageMetadataProviderinutil_test.gowith a local fakeaccounts.google.comDetails
Multiple unit tests made real HTTP calls to container registries (
quay.io,registry-1.docker.io) and identity providers (accounts.google.com), causing test failures when the network was unavailable or slow. This was observed in CI:https://github.com/openshift/hypershift/actions/runs/24573813584/job/71853756467?pr=8247
Jira: https://issues.redhat.com/browse/OCPBUGS-83757
Test plan
make testpasses 100% with no network connectivitymake verifypasses (excluding git-clean check)go vetpasses on all modified packages🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Refactor
Bug Fix