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

Organise unit tests to use pytest fixtures (test_repository.py implementation) #176

Conversation

SeanBryan51
Copy link
Collaborator

No description provided.

@SeanBryan51 SeanBryan51 changed the title Organise unit tests to use pytest fixtures (test_repository.py implementation) Organise unit tests to use pytest fixtures (test_repository.py implementation) Oct 10, 2023
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

Merging #176 (879edc7) into 163-tidy-up-organisation-of-unit-tests (19eff9e) will increase coverage by 14.03%.
The diff coverage is 97.20%.

@@                             Coverage Diff                             @@
##           163-tidy-up-organisation-of-unit-tests     #176       +/-   ##
===========================================================================
+ Coverage                                   46.42%   60.45%   +14.03%     
===========================================================================
  Files                                          25       25               
  Lines                                        1676     1659       -17     
===========================================================================
+ Hits                                          778     1003      +225     
+ Misses                                        898      656      -242     
Files Coverage Δ
tests/conftest.py 92.98% <100.00%> (+19.76%) ⬆️
tests/test_benchcab.py 100.00% <ø> (ø)
tests/test_comparison.py 90.90% <ø> (ø)
tests/test_config.py 1.75% <ø> (ø)
tests/test_fluxsite.py 97.27% <ø> (ø)
tests/test_fs.py 14.28% <ø> (ø)
tests/test_workdir.py 10.71% <ø> (ø)
tests/test_repository.py 97.33% <97.16%> (+93.76%) ⬆️

... and 3 files with indirect coverage changes

Copy link
Collaborator

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

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

It's more a question than a request for changes. I see we aren't cleaning up created files and directories anymore. Is that a choice?

tests/test_repository.py Outdated Show resolved Hide resolved
(internal.SRC_DIR / repo.name / "offline" / "Makefile").touch()
(internal.SRC_DIR / repo.name / "offline" / "parallel_cable").touch()
(internal.SRC_DIR / repo.name / "offline" / "serial_cable").touch()
(internal.SRC_DIR / repo.name / "offline" / "foo.f90").touch()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add a teardown?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I figured all directories under the mock_cwd are removed at the end by the _run_around_tests fixture which is why I haven't been adding a teardown step.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we have a comment at the file level to remind us of that? Happy to have the same comment in all test_* files. But I am sure to have forgotten in a few days otherwise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, we can put a comment the module docstring, e.g:

"""`pytest` tests for `repository.py`.

Note: explicit teardown for generated files and directories are not required as
the working directory used for testing is cleaned up in the `_run_around_tests`
pytest autouse fixture.
"""

tests/test_repository.py Show resolved Hide resolved
tests/test_repository.py Show resolved Hide resolved
tests/test_repository.py Show resolved Hide resolved
tests/test_repository.py Show resolved Hide resolved
tests/test_repository.py Show resolved Hide resolved
tests/test_repository.py Show resolved Hide resolved
Add missing `parametrize()` decorator.
Copy link
Collaborator

@ccarouge ccarouge left a comment

Choose a reason for hiding this comment

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

If you can add the module docstring before merging that would be great.

Add note on cleaning up generated files and directories in the tests.
@SeanBryan51 SeanBryan51 merged commit b3a0fac into 163-tidy-up-organisation-of-unit-tests Oct 11, 2023
4 checks passed
@SeanBryan51 SeanBryan51 deleted the 163-tidy-up-organisation-of-unit-tests-test_repository branch October 11, 2023 03:58
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.

2 participants