-
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
prevent predicatesOrdering
from escaping from UT
#77576
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Huang-Wei 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 |
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.
Thanks, @Huang-Wei
@@ -217,7 +217,7 @@ func TestSelectHost(t *testing.T) { | |||
} | |||
|
|||
func TestGenericScheduler(t *testing.T) { | |||
algorithmpredicates.SetPredicatesOrdering(order) | |||
defer algorithmpredicates.SetPredicatesOrderingDuringTest(order)() |
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.
nit: this is clever, but a bit unreadable. One might think that the whole line is defer
ed and executed after the function returns. Optionally, you can store and defer
the returned function, but it is up to you.
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.
Credit goes to @liggitt :)
defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.XYZ, true)()
@@ -174,6 +174,9 @@ func Ordering() []string { | |||
} | |||
|
|||
// SetPredicatesOrdering sets the ordering of predicates. | |||
// CAVEAT: be cautious with the usage. | |||
// It can cause predicatesOrdering to be modified permanately. | |||
// If your intention is writing a test, the correct way is utils.go#SetPredicatesOrderingDuringTest(). | |||
func SetPredicatesOrdering(names []string) { |
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.
Please remove this function.
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.
Done. PTAL.
- sets `predicatesOrdering` back to original value in UT
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.
/lgtm
Thanks, @Huang-Wei
What type of PR is this?
/kind bug
/kind failing-test
/sig scheduling
/priority important-soon
What this PR does / why we need it:
generic_scheduler_test.go
modifies "predicatesOrdering" in UT without setting it back. This causes modified "predicatesOrdering" to be effective in the entire unit test lifecycle, hence could lead to flaky tests and unexpected behavior. For example, it fails when runningTestPreempt
only:Special notes for your reviewer:
If appropriate, we can remove function
SetPredicatesOrdering()
- there is no usage inside k8s codebase.Does this PR introduce a user-facing change?:
/assign @k82cn @bsalamat