fix flaky functional test - #4325
Conversation
kubevirt-bot
added
release-note-none
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughTwo new exported functions, Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 13 linked repositories, but your current plan allows 0. Analyzed ``, skipped Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/func-tests/conversion_test.go (1)
73-130: ⚡ Quick winMake the patch always flip selected gates away from defaults.
Right now values are hardcoded (
beta=false,alpha=true). If a selected gate already has that default (and only one gate is available), this can pass without proving mutation/conversion behavior. Consider deriving patch values as!default.Suggested change
- var patchFGs []string + var patchFGs []string + expectedFGs := map[string]bool{} + defaultFGs := featuregates.HyperConvergedFeatureGates{} if betaFG != "" { + newVal := !defaultFGs.IsEnabled(betaFG) GinkgoLogr.Info("found a beta feature gate with a field in v1beta1 API version", "name", betaFG) - patchFGs = []string{fmt.Sprintf(`%q: false`, betaFG)} + patchFGs = append(patchFGs, fmt.Sprintf(`%q: %t`, betaFG, newVal)) + expectedFGs[betaFG] = newVal } else { GinkgoLogr.Info("no beta feature gate defined in v1beta1 API version") } if alphaFG != "" { + newVal := !defaultFGs.IsEnabled(alphaFG) GinkgoLogr.Info("found an alpha feature gate with a field in v1beta1 API version", "name", alphaFG) - patchFGs = append(patchFGs, fmt.Sprintf(`%q: true`, alphaFG)) + patchFGs = append(patchFGs, fmt.Sprintf(`%q: %t`, alphaFG, newVal)) + expectedFGs[alphaFG] = newVal } else { GinkgoLogr.Info("no alpha feature gate defined in v1beta1 API version") } @@ - if betaFG != "" { - Expect(hcv1.Spec.FeatureGates.IsEnabled(betaFG)).To(BeFalseBecause("the %q beta feature gate was disabled using v1beta1 API. it is expected to be 'false' in v1, but it's not", betaFG)) + if betaFG != "" { + Expect(hcv1.Spec.FeatureGates.IsEnabled(betaFG)). + To(Equal(expectedFGs[betaFG]), "beta gate %q did not match patched value", betaFG) } - if alphaFG != "" { - Expect(hcv1.Spec.FeatureGates.IsEnabled(alphaFG)).To(BeTrueBecause("the %q alpha feature gate was enabled using v1beta1 API. it is expected to be 'true' in v1, but it's not", alphaFG)) + if alphaFG != "" { + Expect(hcv1.Spec.FeatureGates.IsEnabled(alphaFG)). + To(Equal(expectedFGs[alphaFG]), "alpha gate %q did not match patched value", alphaFG) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/func-tests/conversion_test.go` around lines 73 - 130, The patch values for the feature gates are hardcoded (betaFG set to false and alphaFG set to true) without checking their current default values. This means if a selected gate already defaults to that value, the test could pass without actually testing mutation behavior. Before building the patchFGs slice where the fmt.Sprintf calls with hardcoded false and true values are made, first retrieve the current values of the HyperConverged object to determine the default state of each feature gate, then invert those values when constructing the patch strings to ensure the test always flips the gates away from their defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/func-tests/conversion_test.go`:
- Around line 73-130: The patch values for the feature gates are hardcoded
(betaFG set to false and alphaFG set to true) without checking their current
default values. This means if a selected gate already defaults to that value,
the test could pass without actually testing mutation behavior. Before building
the patchFGs slice where the fmt.Sprintf calls with hardcoded false and true
values are made, first retrieve the current values of the HyperConverged object
to determine the default state of each feature gate, then invert those values
when constructing the patch strings to ensure the test always flips the gates
away from their defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e4467e4-c4d8-4ca6-a636-4a94f269ccb6
📒 Files selected for processing (2)
pkg/featuregatedetails/feature_gates.gotests/func-tests/conversion_test.go
Coverage Report for CI Build 27905282496Coverage increased (+0.02%) to 80.452%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/featuregatedetails/feature_gates_test.go`:
- Around line 86-87: The Context block in the test has a mislabeled name that
does not match its actual test content. Rename the Context from
"ListBetaFeatureGates" to "ListAlphaFeatureGates" to accurately reflect that the
test block validates the ListAlphaFeatureGates functionality, not
ListBetaFeatureGates. This ensures the test output is clear and prevents
confusion for future developers reusing this test structure.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 409ca564-0013-4487-b014-b01c2cf2a4fb
📒 Files selected for processing (3)
pkg/featuregatedetails/feature_gates.gopkg/featuregatedetails/feature_gates_test.gotests/func-tests/conversion_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/featuregatedetails/feature_gates.go
- tests/func-tests/conversion_test.go
|
hco-e2e-upgrade-operator-sdk-sno-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-aws, ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-operator-sdk-sno-aws, ci/prow/hco-e2e-upgrade-operator-sdk-aws, ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure, ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-aws 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 kubernetes-sigs/prow repository. |
|
/override-bot |
|
hco-e2e-upgrade-operator-sdk-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-upgrade-operator-sdk-aws 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 kubernetes-sigs/prow repository. |
|
/retest |
|
hco-e2e-upgrade-prev-operator-sdk-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-prev-operator-sdk-aws 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 kubernetes-sigs/prow repository. |
| "fg6": {Name: "fg4", Phase: featuregates.PhaseDiscontinued}, | ||
| "alpha2": {Name: "alpha2", Phase: featuregates.PhaseAlpha}, | ||
| "beta2": {Name: "beta2", Phase: featuregates.PhaseBeta}, | ||
| "beta3": {Name: "beta2", Phase: featuregates.PhaseBeta}, |
There was a problem hiding this comment.
| "beta3": {Name: "beta2", Phase: featuregates.PhaseBeta}, | |
| "beta3": {Name: "beta3", Phase: featuregates.PhaseBeta}, |
The "naively read HCO in v1beta1 format" test is flaky. if the featureGates is an empty array (rather than nil), the comparison between the the fetched and the converted CRs fails, because after the conversion, the array is nil. This is not a bug in the conversion! To fix, we makes sure the array is nil before running the test. In addition, the "should allow set fields in HyperConverged v1beta1" will break if we'll change the specific FGs we're using there. This commit also modified this test to dynamically choose the feature gates for the test. Signed-off-by: Nahshon Unna Tsameret <nunnatsa@redhat.com>
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: avlitman 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 |
kubevirt-bot
added
the
approved
|
@nunnatsa: The following tests 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. |
|
hco-e2e-consecutive-operator-sdk-upgrades-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure, ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-operator-sdk-gcp, ci/prow/hco-e2e-operator-sdk-sno-azure, ci/prow/hco-e2e-upgrade-operator-sdk-sno-aws, ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure 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 kubernetes-sigs/prow repository. |
|
hco-e2e-kv-smoke-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure 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 kubernetes-sigs/prow repository. |
|
5 participants
What this PR does / why we need it:
The "naively read HCO in v1beta1 format" test is flaky. if the featureGates is an empty array (rather than nil), the comparison between the the fetched and the converted CRs fails, because after the conversion, the array is nil. This is not a bug in the conversion!
To fix, we makes sure the array is nil before running the test.
In addition, the "should allow set fields in HyperConverged v1beta1" will break if we'll change the specific FGs we're using there. This commit also modified this test to dynamically choose the feature gates for the test.
Jira Ticket:
Release note: