test: add test that verifies subsequent tasks are NEVER_ATTEMPTED when previous action is canceled #437
Conversation
YutongLi291
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
test/e2e/test_job_submissions.py
Outdated
| assert is_job_started_with_sessions(job) | ||
|
|
||
| # Wait some time for the second step (which sleeps for 2 minutes) to start | ||
| time.sleep(20) |
There was a problem hiding this comment.
Instead of sleeping here, it may be worth just waiting on the second step to switch to RUNNING? That way if it starts sooner, the test can complete faster.
There was a problem hiding this comment.
That's true, but that's also more API calls. There's a tradeoff, and I believe that 20 seconds is not a big time wait to justify the extraneous API calls.
There was a problem hiding this comment.
I see the sleep command more as a last resort. It guarantees to slow down all testing, especially when testing new changes before PR. Between these two sleeps, you're adding thirty seconds to every full test attempt, which already takes an extremely long time to complete. If I'm not mistaken, most people make sure their changes work with the full suite more than once before creating a PR, that's at least a minute to every future change people try to merge.
test/e2e/test_job_submissions.py
Outdated
| @@ -406,6 +406,9 @@ def sessions_exist(current_job: Job) -> bool: | |||
|
|
|||
| LOG.info(f"Job result: {job}") | |||
|
|
|||
| # Wait until the envExit runs as well | |||
| time.sleep(10) | |||
There was a problem hiding this comment.
Is anything actually happening on exit that we need to sleep this long? I see that it waits for the job to complete above. Should that function not already make sure that the job has finished it's calls before returning?
There was a problem hiding this comment.
The job is considered completed even without running the envExit and envExit is not required to finish for the job to finish.
…n previous action is canceled Signed-off-by: Yutong Li <[email protected]>
|
|
What was the problem/requirement? (What/Why)
The scheduler doesn’t pipeline more than one task run in a session until the first task run in that session has completed.
We should verify that this behaviour is as expected, and that if the task run is not completed, other tasks should not have been attempted.
What was the solution? (How)
Add an E2E test that verifies that when a task is canceled during its run, the subsequent tasks are in
NEVER_ATTEMPTEDstatus.Also edited an existing test that verifies
CANCELEDactions that other subsequent actions areNEVER_ATTEMPTEDWhat is the impact of this change?
Better testing to verify worker behaviour is as expected when task is canceled, that following tasks are NEVER_ATTEMPTED
How was this change tested?
Was this change documented?
No
Is this a breaking change?
No
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.