-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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
Integration tests: Migrate scheduler perf to the integration suite, s… #32520
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,3 +73,6 @@ cd kubernetes/test/component/scheduler/perf | |
<!-- BEGIN MUNGE: GENERATED_ANALYTICS --> | ||
[![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/test/component/scheduler/perf/README.md?pixel)]() | ||
<!-- END MUNGE: GENERATED_ANALYTICS --> | ||
|
||
|
||
[![Analytics](https://kubernetes-site.appspot.com/UA-36037335-10/GitHub/test/integration/scheduler_perf/README.md?pixel)]() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems unneded to me. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,7 +44,7 @@ import ( | |
// Notes on rate limiter: | ||
// - client rate limit is set to 5000. | ||
func mustSetupScheduler() (schedulerConfigFactory *factory.ConfigFactory, destroyFunc func()) { | ||
framework.DeleteAllEtcdKeys() | ||
// framework.DeleteAllEtcdKeys() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why did this get commented out? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needed to disable for compilation. I considered deleting it but, clearly it was originally there for some purpose. Once these are working again we can determine wether or not we want to re-enable etcd deletion. As of now, a new etc instance that would typically get started by the test wrapper makes this line not necessary unless running on a persistent instance There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a comment explaining why it's commented out / under what conditions it should get uncommented? otherwise 3 months from now we're not going to understand why it's there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pretty sure we can simply remove it. |
||
|
||
var m *master.Master | ||
masterConfig := framework.NewIntegrationTestMasterConfig() | ||
|
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.
Actually, I realized that I don't understand why you are removing this init() function. Can you please explain? [What you did seems more complicated.]
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.
@wojtek-t replied below... removing
init()
so thatt.Skip(..)
works. The broader reason to remove it is in the comment below.