-
Notifications
You must be signed in to change notification settings - Fork 38.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ensure feature gate changes don't escape unit tests #71100
Conversation
/assign @msau42 |
/milestone v1.13 |
9c95e76
to
afa079e
Compare
@msau42 how confident are we in getting this in by Code freeze tomorrow? |
/assign @smarterclayton |
rc := tests() | ||
|
||
finalSet := fmt.Sprint(gate) | ||
if finalSet != originalSet { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow up for someone - we should be able to read the feature gates instead of having ot parse from string.
For the follow up verbally confirmed that gate would also check main_test.go exists |
opened #71108 to track follow-up tasks for this... I'm on the fence about a way to export a map from a feature gate |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, smarterclayton The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
c761796
to
733dd9d
Compare
New changes are detected. LGTM label has been removed. |
rebased, fixed up unit tests added in https://github.com/kubernetes/kubernetes/pull/68635/files#diff-bda636cade29666ccecd6df250c834b8R87 last night, retagging |
/retest |
/lgtm |
/retest |
1 similar comment
/retest |
What type of PR is this?
/kind bug
What this PR does / why we need it:
while reviewing tests in #65829, we realized some unit tests originally set up to test behavior when an alpha gate was enabled, were leaking changes to feature gates out of their packages by attempting to reset them to the old, default, disabled values.
This adds a package-level check that feature gates are not modified once tests are complete in a package and fixes existing tests that leaked feature gate changes beyond their package. This is critical that our unit tests are working with default feature gate values.
Will follow up with validation scripts that ensure this check is done in any package setting feature gates in test files.
fixes #71091