[ENH] Include tests for the queue command #110

2022-11-16T09:43:27Z

synchon commented

2022-11-16 09:43:27 +00:00

(Migrated from github.com)

julearn.api.functions.queue and julearn.api.functions._queue_condor are currently not tested.

The functionality should be tested. At least that the YAML file is created, the submit file is somehow correct, the executable file is there, etc etc.

To consider:

The logs directory should exist/be created. Otherwise the jobs will be on hold forever.

It will be ideal if we can check the validity of the submit file according to htcondor syntax.

`julearn.api.functions.queue` and `julearn.api.functions._queue_condor` are currently not tested. The functionality should be tested. At least that the YAML file is created, the submit file is somehow correct, the executable file is there, etc etc. To consider: - The logs directory should exist/be created. Otherwise the jobs will be on hold forever. It will be ideal if we can check the validity of the submit file according to htcondor syntax.

github-actions[bot] commented

2022-11-16 09:48:37 +00:00

(Migrated from github.com)

PR Preview Action v1.2.0
Preview removed because the pull request was closed.
2022-11-17 19:31 UTC

[PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.2.0 :---: Preview removed because the pull request was closed. 2022-11-17 19:31 UTC

codecov[bot] commented

2022-11-16 09:55:03 +00:00

(Migrated from github.com)

Codecov Report

Merging #110 (5bdc89f) into main (c3d29e8) will increase coverage by 4.07%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main     #110      +/-   ##
==========================================
+ Coverage   91.01%   95.08%   +4.07%     
==========================================
  Files          57       57              
  Lines        2260     2260              
  Branches      424      424              
==========================================
+ Hits         2057     2149      +92     
+ Misses        163       71      -92     
  Partials       40       40

Flag	Coverage Δ
junifer	`95.07% <75.00%> (+4.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
junifer/api/functions.py	`97.98% <75.00%> (+61.74%)`	⬆️

# [Codecov](https://codecov.io/gh/juaml/junifer/pull/110?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) Report > Merging [#110](https://codecov.io/gh/juaml/junifer/pull/110?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (5bdc89f) into [main](https://codecov.io/gh/juaml/junifer/commit/c3d29e8e5723cf6e4b752acaac1911787037a963?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) (c3d29e8) will **increase** coverage by `4.07%`. > The diff coverage is `75.00%`. [![Impacted file tree graph](https://codecov.io/gh/juaml/junifer/pull/110/graphs/tree.svg?width=650&height=150&src=pr&token=5H21JuZXMw&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml)](https://codecov.io/gh/juaml/junifer/pull/110?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) ```diff @@ Coverage Diff @@ ## main #110 +/- ## ========================================== + Coverage 91.01% 95.08% +4.07% ========================================== Files 57 57 Lines 2260 2260 Branches 424 424 ========================================== + Hits 2057 2149 +92 + Misses 163 71 -92 Partials 40 40 ``` | Flag | Coverage Δ | | |---|---|---| | junifer | `95.07% <75.00%> (+4.07%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/juaml/junifer/pull/110?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml) | Coverage Δ | | |---|---|---| | [junifer/api/functions.py](https://codecov.io/gh/juaml/junifer/pull/110/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=juaml#diff-anVuaWZlci9hcGkvZnVuY3Rpb25zLnB5) | `97.98% <75.00%> (+61.74%)` | :arrow_up: |

fraimondo (Migrated from github.com) requested changes

2022-11-16 10:14:27 +00:00

fraimondo (Migrated from github.com) left a comment

Can you add to the queue test a check for the YAML file? It should have all the fields from the original config but the queue section.

Can you also add a test for _queue_condor checking that the:

DAG file exists
submit file exist,
that the DAG files has as many run lines as elements
mem, cpus and disk parameter has effect
if collect is true, then the DAG should have a job collect. collect submit file should be present anyways
extra preamble parameter has effect
Check for submit = True: should give a File not found error or something like that

Can you add to the `queue` test a check for the YAML file? It should have all the fields from the original config but the `queue` section. Can you also add a test for `_queue_condor` checking that the: * DAG file exists * submit file exist, * that the DAG files has as many run lines as elements * mem, cpus and disk parameter has effect * if collect is true, then the DAG should have a job collect. collect submit file should be present anyways * extra preamble parameter has effect * Check for submit = True: should give a File not found error or something like that

junifer/api/tests/test_functions.py

					
				@ -151,0 +356,4 @@

				    monkeypatch: pytest.MonkeyPatch,

				    caplog: pytest.LogCaptureFixture,

				    elements: Union[str, List[Union[str, Tuple[str]]], Tuple[str]],

				) -> None:

Can you assert that the file is present in the jobdir?

junifer/api/tests/test_functions.py

					
				@ -151,0 +374,4 @@

				    with monkeypatch.context() as m:

				        m.chdir(tmp_path)

				        with caplog.at_level(logging.INFO):

				            queue(

Can you add elements that are tuples? Like [("sub-001", "ses-001"),("sub-001", "ses-002")]

Can you add elements that are tuples? Like `[("sub-001", "ses-001"),("sub-001", "ses-002")]`

synchon (Migrated from github.com) reviewed

2022-11-16 18:37:29 +00:00

junifer/api/tests/test_functions.py

					
				@ -151,0 +356,4 @@

				    monkeypatch: pytest.MonkeyPatch,

				    caplog: pytest.LogCaptureFixture,

				    elements: Union[str, List[Union[str, Tuple[str]]], Tuple[str]],

				) -> None:

Done.

synchon (Migrated from github.com) reviewed

2022-11-16 18:44:35 +00:00

junifer/api/tests/test_functions.py

					
				@ -151,0 +374,4 @@

				    with monkeypatch.context() as m:

				        m.chdir(tmp_path)

				        with caplog.at_level(logging.INFO):

				            queue(