-
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
Decouple yaml based integration test from legacy test #89106
Conversation
/test pull-kubernetes-conformance-kind-ipv6-parallel |
/test pull-kubernetes-verify |
/test pull-kubernetes-verify |
/test pull-kubernetes-e2e-kind-ipv6 |
/test pull-kubernetes-kubemark-e2e-gce-big |
I think we can remove the legacy way to run scheduler perf directly, or mark the legacy way as deprecated? |
@chendave It seems that what the PR does is different from its description? It only moves a function and some variables to the util file. |
I think there are two approaches to run those benchmarks, one is based on the yaml template, and another one is based on inline test spec, they are peer-to-peer instead of one approach depends on another one. But current code base has some method and consts defined in the legacy inline test spec, this doesn't make sense in my opinion, simply because I should be able to run the benchmark without any dependencies in the legacy code (which eventually should be removed in the future). |
@hex108 , thank you for the review and comment! hope you get my points. |
remove this file: test/integration/scheduler_perf/scheduler_bench_test.go (eventually, this should be deleted in the future). and then run the scheduler perf with the new way, you will find it will fail with some constants / method missing. |
/test pull-kubernetes-verify |
2 similar comments
/test pull-kubernetes-verify |
/test pull-kubernetes-verify |
@hex108 how about deprecate those test suit? at least the new one should not depends on the old one (see my first PR). |
@ahg-g @liu-cong @ingvagabund, is this PR resolve part of what you discussed in #88195 to replace legacy BenchmarkScheduling suite with new BenchmarkPerfScheduling suite? I suppose there is some change needed for test-infra/dashboard as well? |
/test pull-kubernetes-e2e-kind |
It makes sense to do |
The refactor makes it possible to run those integration test without the code dependencies in the legacy approach. This is somehow align with what you said below.
|
@chendave oh, I see what you mean. The code you moved from |
- Move utilities or constants out so that both of them should be able to run independently. - Rename the legacy test so that it can eventually be deleted when the perf dash changes is done
/lgtm |
/assign |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, chendave 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 |
/test pull-kubernetes-e2e-gce |
/retest |
/sig scheduling |
to run independently.
perf dash changes is done
There are two approaches to run
scheduler_perf
testcases with different ways to define the test spec, both of them could be able to run independently.The second one becomes duplicated along with the merge of defining the test spec in the yaml file. There is no need to maintain both of them in the code base, this change refactor the code to decouple one from the other and drop the support of the legacy way to run the
scheduler_perf
.What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: