Skip to content

rewrite testing across clusters using fixtures#533

Merged
guillaumeeb merged 5 commits into
dask:mainfrom
riedel:rewrite_env_testing_using_fixtures
Nov 24, 2021
Merged

rewrite testing across clusters using fixtures#533
guillaumeeb merged 5 commits into
dask:mainfrom
riedel:rewrite_env_testing_using_fixtures

Conversation

@riedel

@riedel riedel commented Nov 1, 2021

Copy link
Copy Markdown
Member
  • removes considerably more lines than added
  • rigorously tests accross all scheduler implementations: tests increase from 168 items to 231 items (not all are actually tested because there is no seperate CI for some)
  • move logic for paramaterized testing across multiple schedulers to two fixtures (one that runs locally and one that requires the CI environment): this allows adding new schedulers easily
  • add additional required disk argument for HTCondor to fixture using a temporary config default
  • test security on all clusters and add xfail for security on HTCondor and Slurm CI
  • modify the LocalCluster job_script to use a shebang line in order to behave equally to "real" schedulers (instead of "parsing" the jobscript and "emulating" a shell interpreter)

Please not that this conflicts and is not in sync with @lesteve's #353 . But it might be easier now since #521 was "fixed" .

xfail security
align LocalCluster api for unified testing
move job unrelated test functions from test_job and to test_jobqueue_core for now
@riedel

riedel commented Nov 1, 2021

Copy link
Copy Markdown
Member Author

IMHO this makes testing alot easier to read. It also reduces the lines of code necessary considerably.

@riedel riedel changed the title rewrite testing using fixtures rewrite testing across clusters using fixtures Nov 1, 2021
move htcondor defaults to fixture
revert move of docstring check (to make diff minimal)
test_job now uses fixture
@riedel riedel added CI Continuous Integration tools all job schedulers labels Nov 7, 2021
@riedel riedel requested review from guillaumeeb and lesteve November 7, 2021 13:44

@riedel riedel left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I tried to add a few explanations

Comment thread dask_jobqueue/core.py Outdated
Comment thread dask_jobqueue/tests/test_job.py
Comment thread dask_jobqueue/tests/test_jobqueue_core.py
Comment thread dask_jobqueue/tests/test_jobqueue_core.py
Comment thread dask_jobqueue/tests/test_jobqueue_core.py
add link to issue in xfail
@riedel riedel force-pushed the rewrite_env_testing_using_fixtures branch from 34fffe4 to 7656084 Compare November 16, 2021 00:40
@riedel riedel requested a review from andersy005 November 16, 2021 00:59
Comment thread dask_jobqueue/tests/test_jobqueue_core.py Outdated

@guillaumeeb guillaumeeb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks really neat!!

I just have a few questions and clarification, and a bit reluctance changing the submission script template.

Otherwise this would be a much welcomed change!

Comment thread conftest.py Outdated
Comment thread conftest.py Outdated
Comment thread dask_jobqueue/core.py Outdated
Comment thread dask_jobqueue/jobqueue.yaml Outdated
Comment thread dask_jobqueue/local.py Outdated
Comment thread conftest.py
Comment thread conftest.py
document fixtures
remove leftovers and inconsistencies
rename xfail_ci to xfail_env for consistency
@riedel

riedel commented Nov 22, 2021

Copy link
Copy Markdown
Member Author

@guillaumeeb , thanks for reviewing and catching some issues, I tried to incorporate them. I removed the changes to local.py and core.py as they are debatable and can go in a seperate PR . Hope no concerns are left. Added some docstrings to the fixtures.

@riedel riedel left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

all your comments should be addressed ?!?

Comment thread dask_jobqueue/local.py Outdated
Comment thread conftest.py Outdated
Comment thread conftest.py
Comment thread dask_jobqueue/jobqueue.yaml Outdated
@guillaumeeb guillaumeeb merged commit def2643 into dask:main Nov 24, 2021
@guillaumeeb

Copy link
Copy Markdown
Member

Thanks once more @riedel !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

all job schedulers CI Continuous Integration tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants