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

Tests and updates #92

Merged
merged 6 commits into from
Mar 19, 2024
Merged

Tests and updates #92

merged 6 commits into from
Mar 19, 2024

Conversation

gpetretto
Copy link
Contributor

This PR introduces several new tests and some tools to run then (e.g. to execute the test on CLI commands and options in the runner to run a single job).
Some of the new tests are under the integration folder. Others are under the new db folder. They do not require the docker containers, but need a mongodb database. Locally they are executed with a standard mongodb installation, or, if not available, with pymongo_inmemory. On github the mongodb should be the local_mongodb service.

Writing the tests some minor bugs were identified and fixed.

One notable change is that now the Runner.run does not call the Runner.cleanup method anymore and the caller should call it. It was more convenient for tests, but it is also more reasonable for a couple of reasons: 1) the code may have failed before getting to the try...finally, so it was not guaranteed that the cleanup was really called 2) the caller may need control over the Runner object (e.g. may be reused after calling run)

@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2024

Codecov Report

Attention: Patch coverage is 37.27811% with 106 lines in your changes are missing coverage. Please review.

Project coverage is 49.78%. Comparing base (3a6ee13) to head (676d75c).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop      #92      +/-   ##
===========================================
+ Coverage    44.78%   49.78%   +5.00%     
===========================================
  Files           42       43       +1     
  Lines         4615     4744     +129     
  Branches       958      988      +30     
===========================================
+ Hits          2067     2362     +295     
+ Misses        2359     2149     -210     
- Partials       189      233      +44     
Files Coverage Δ
src/jobflow_remote/cli/jf.py 74.19% <ø> (+40.86%) ⬆️
src/jobflow_remote/cli/project.py 44.61% <100.00%> (ø)
src/jobflow_remote/config/base.py 81.81% <ø> (ø)
src/jobflow_remote/config/manager.py 43.70% <100.00%> (ø)
src/jobflow_remote/cli/formatting.py 11.39% <0.00%> (ø)
src/jobflow_remote/cli/job.py 20.84% <0.00%> (ø)
src/jobflow_remote/cli/utils.py 31.00% <0.00%> (+0.74%) ⬆️
src/jobflow_remote/utils/db.py 70.53% <60.00%> (-1.03%) ⬇️
src/jobflow_remote/cli/runner.py 24.00% <0.00%> (-0.40%) ⬇️
src/jobflow_remote/jobs/jobcontroller.py 38.48% <25.00%> (+1.78%) ⬆️
... and 5 more

... and 9 files with indirect coverage changes

@gpetretto
Copy link
Contributor Author

Tests now pass. For future reference, I had to add the explicit --cov-config pyproject.toml option to the pytest command. Apparently this is a common issue: pytest-dev/pytest-cov#237.

@ml-evs @davidwaroquiers if you are fine with the structure of the tests I will also update the developer documentation accordingly.

I admit I was hoping to get an increase in the coverage higher than just 5%. I am not sure if it is counting correctly the tests on the CLI and I suppose all the processes that run "remotely" will not be considered either.

@ml-evs ml-evs self-requested a review March 13, 2024 16:13
@ml-evs
Copy link
Member

ml-evs commented Mar 13, 2024

I admit I was hoping to get an increase in the coverage higher than just 5%. I am not sure if it is counting correctly the tests on the CLI and I suppose all the processes that run "remotely" will not be considered either.

I dug into this a bit and I'm confused, codecov reports 21% coverage of jobflow_remote.cli.job as it stands, but when I run the same test invocation locally, I get 61%, which sounds more realistic:

$ pytest --ignore tests/integration --cov=jobflow_remote --cov-config pyproject.toml --cov-report=html
...
$ coverage report
Name                                       Stmts   Miss Branch BrPart  Cover   Missing
--------------------------------------------------------------------------------------
src/jobflow_remote/cli/admin.py               69      2     36      3    95%   65->76, 104-109, 137->163, 196->223
src/jobflow_remote/cli/execution.py           13      2      4      0    88%   30, 85
src/jobflow_remote/cli/flow.py                87     25     50      4    67%   93->100, 141->150, 173-176, 189, 234-268
src/jobflow_remote/cli/formatting.py         158     19     70     16    83%   48, 63-64, 68-70, 140, 148, 155, 159, 193->198, 200->217, 205, 211, 213, 225->227, 229-230, 235->237, 239-242
src/jobflow_remote/cli/jf.py                  33      6     12      4    73%   36, 76, 79, 90-95
src/jobflow_remote/cli/jfr_typer.py           23      0      4      1    96%   20->26
src/jobflow_remote/cli/job.py                256     87    120     20    67%   126->exit, 128, 137-142, 179, 274->277, 420-422, 518, 525-572, 727, 798->787, 802, 835->845, 841->845, 846->832, 849-855, 889, 896-918, 945->947, 949, 962, 969-987
...

codecov even reports 0% on the jobflow_remote.testing.cli module, so something is clearly going wrong.

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Again, nothing jumps out here to comment on, the test additions look great (and its a shame coverage seems to be misreported -- hopefully possible to fix this in the future). All good on my side to do a release.

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Again, nothing jumps out here to comment on, the test additions look great (and its a shame coverage seems to be misreported -- hopefully possible to fix this in the future). All good on my side to do a release.

@gpetretto gpetretto merged commit 7993742 into develop Mar 19, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants