Skip to content
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

Move test_stop_running into test_job_queue #3709

Merged
merged 1 commit into from
Aug 5, 2022

Conversation

dafeda
Copy link
Contributor

@dafeda dafeda commented Aug 5, 2022

Pre review checklist

  • Added appropriate release note label
  • PR title captures the intent of the changes, and is fitting for release notes.
  • Commit history is consistent and clean, in line with the contribution guidelines.

Adding labels helps the maintainers when writing release notes. This is the list of release note labels.

@dafeda dafeda self-assigned this Aug 5, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2022

Codecov Report

Merging #3709 (b5c15bb) into main (0079c8e) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3709      +/-   ##
==========================================
- Coverage   63.34%   63.33%   -0.02%     
==========================================
  Files         597      597              
  Lines       46243    46243              
  Branches     4144     4144              
==========================================
- Hits        29293    29288       -5     
- Misses      15662    15666       +4     
- Partials     1288     1289       +1     
Impacted Files Coverage Δ
src/res/job_queue/forward_model_status.py 88.60% <0.00%> (-5.07%) ⬇️
src/libres/lib/res_util/block_fs.cpp 67.34% <0.00%> (-0.41%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

The test has nothing to do with EnkfSimulationRunner.
@dafeda dafeda changed the title Commitin from unittest import mock Aug 5, 2022
@dafeda dafeda changed the title from unittest import mock Move test_stop_running into test_job_queue Aug 5, 2022
@dafeda dafeda added the release-notes:maintenance Automatically categorise as maintenance change in release notes label Aug 5, 2022
@dafeda dafeda marked this pull request as ready for review August 5, 2022 07:21
Comment on lines +371 to +381
for i in range(5):
assert job_list[i].status == JobStatusType.JOB_QUEUE_DONE
assert queue.snapshot()[i] == str(JobStatusType.JOB_QUEUE_DONE)

for i in range(5, 8):
assert job_list[i].status == JobStatusType.JOB_QUEUE_FAILED
assert queue.snapshot()[i] == str(JobStatusType.JOB_QUEUE_FAILED)

for i in range(8, 10):
assert job_list[i].status == JobStatusType.JOB_QUEUE_RUNNING
assert queue.snapshot()[i] == str(JobStatusType.JOB_QUEUE_RUNNING)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a better assert:

Suggested change
for i in range(5):
assert job_list[i].status == JobStatusType.JOB_QUEUE_DONE
assert queue.snapshot()[i] == str(JobStatusType.JOB_QUEUE_DONE)
for i in range(5, 8):
assert job_list[i].status == JobStatusType.JOB_QUEUE_FAILED
assert queue.snapshot()[i] == str(JobStatusType.JOB_QUEUE_FAILED)
for i in range(8, 10):
assert job_list[i].status == JobStatusType.JOB_QUEUE_RUNNING
assert queue.snapshot()[i] == str(JobStatusType.JOB_QUEUE_RUNNING)
result = [(job.status, item) for (job, item) in zip(job_list, queue.snapshot())]
assert result == [
(JobStatusType(name="JOB_QUEUE_DONE", value=32), 0),
(JobStatusType(name="JOB_QUEUE_DONE", value=32), 1),
(JobStatusType(name="JOB_QUEUE_DONE", value=32), 2),
(JobStatusType(name="JOB_QUEUE_DONE", value=32), 3),
(JobStatusType(name="JOB_QUEUE_DONE", value=32), 4),
(JobStatusType(name="JOB_QUEUE_FAILED", value=8192), 5),
(JobStatusType(name="JOB_QUEUE_FAILED", value=8192), 6),
(JobStatusType(name="JOB_QUEUE_FAILED", value=8192), 7),
(JobStatusType(name="JOB_QUEUE_RUNNING", value=16), 8),
(JobStatusType(name="JOB_QUEUE_RUNNING", value=16), 9),
]

know you are only moving this code, so do it if you agree. The advantage is one assert instead of many, so the feedback on failure is hopefully better.

@dafeda dafeda merged commit 1b771fb into equinor:main Aug 5, 2022
@dafeda dafeda deleted the test_stop_long_running branch August 5, 2022 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:maintenance Automatically categorise as maintenance change in release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants