Improve E2E tests for the gang-scheduling #1801
Conversation
Signed-off-by: Yuki Iwai <[email protected]>
Pull Request Test Coverage Report for Build 4907867447
💛 - Coveralls |
|
/assign @johnugeorge @nagar-ajay |
| if not client.is_job_created(name, namespace, job_kind): | ||
| raise Exception(f"{job_kind} should be in Created condition") | ||
|
|
||
| # Job shouldn't have a Running condition. | ||
| if client.is_job_running(name, namespace, job_kind): |
There was a problem hiding this comment.
Will this be flaky? What if job gets into running state?
There was a problem hiding this comment.
In that case, the test should be failed since we pass an unschedulable job to def verify_unschedulable_job_e2e.
There was a problem hiding this comment.
@tenzen-y Sorry for the late response.
I agree with the behavior, that if we're passing an unschedulable job and it has a running state then our test should fail.
My doubt is, are we sure that the job we're passing is unschedulable? Because in my testing the job had two states (created and running). If the job is unschedulable then it shouldn't have running state, right?
There was a problem hiding this comment.
My doubt is, are we sure that the job we're passing is unschedulable?
@nagar-ajay Ah, I see.
In the previous test, we verify the number of Conditions. So, If the Job has Created=true and Running=false, this test unintentionally fails.
In this PR, the test would be passed in that case (Created=true and Running=false).
Does that make sense?
There was a problem hiding this comment.
Let me summarize for @johnugeorge.
In the previous Implementation, tests go to fail if the Job has Created=false OR Has(Running).
In this implementation, tests go to fail only if Job has Created=false OR Running=true.
|
Thanks for fixing the issue. The changes looks good to me. |
|
Thanks @tenzen-y for explaining |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johnugeorge, tenzen-y 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 |
Pull Request Test Coverage Report for Build 4907867447
💛 - Coveralls |
What this PR does / why we need it:
In E2E, after we created Jobs with unschedulable configurations, we verify the number of conditions is 1 when using scheduler plugins for gang scheduling.
But, the test seems flaky, as reported in #1779.
So, I modified the test to verify whether unschedulable Jobs have a
Createdcondition and don't have aRunningcondition instead of the current way.Which issue(s) this PR fixes (optional, in
Fixes #<issue number>, #<issue number>, ...format, will close the issue(s) when PR gets merged):Fixes #1779
Checklist: