Organise unit tests to use pytest fixtures #172

SeanBryan51 · 2023-10-06T01:01:36Z

Unit tests have an excessive amount of repeated test setup code, making unit tests harder to maintain and read. Using pytest fixtures to setup each test would remove needless code repetition and also prevents the possibility of state leaking into each test case.

Organise the tests so that for each function func, we have a test class TestFunc with each method of TestFunc containing a single test (success or failure case).

Use pytest's parametrize() feature for testing different levels of verbosity.

Remove usage of mock_cwd fixture unless it is required for a test to pass.

Fixes #163

codecov · 2023-10-06T01:05:56Z

Codecov Report

Merging #172 (107cc86) into main (f083d3e) will decrease coverage by 1.48%.
Report is 1 commits behind head on main.
The diff coverage is 97.54%.

@@            Coverage Diff             @@
##             main     #172      +/-   ##
==========================================
- Coverage   88.58%   87.10%   -1.48%     
==========================================
  Files          26       25       -1     
  Lines        1603     1613      +10     
==========================================
- Hits         1420     1405      -15     
- Misses        183      208      +25

Files	Coverage Δ
tests/test_benchcab.py	`100.00% <100.00%> (ø)`
tests/test_config.py	`100.00% <100.00%> (ø)`
tests/test_fs.py	`100.00% <100.00%> (ø)`
tests/test_pbs.py	`100.00% <100.00%> (ø)`
tests/test_subprocess.py	`100.00% <100.00%> (ø)`
tests/test_workdir.py	`100.00% <100.00%> (ø)`
tests/conftest.py	`92.98% <91.83%> (-7.02%)`	⬇️
tests/test_comparison.py	`90.90% <90.00%> (-9.10%)`	⬇️
tests/test_repository.py	`97.33% <97.16%> (-2.67%)`	⬇️
tests/test_fluxsite.py	`97.27% <97.12%> (-2.73%)`	⬇️

... and 2 files with indirect coverage changes

SeanBryan51 · 2023-10-06T01:06:11Z

I have reorganised a few tests files just to see what it looks like. @ccarouge @bschroeter it would be great if I can get your thoughts before I push on further.

ccarouge

I think it looks better than before.

But considering the amount of changes, do we have to do all the tests in the same PR? It might make it very very hard to review. Could we do one module per PR or only a couple at a time?

ccarouge · 2023-10-06T01:38:59Z

tests/test_comparison.py

-    )
+    @pytest.fixture
+    def bitwise_cmp_dir(self, mock_cwd):
+        _bitwise_cmp_dir = mock_cwd / internal.FLUXSITE_BITWISE_CMP_DIR


With the mock_cwd fixture, it means you can use a relative path here (internal.FLUXSITE_BITWISE_CMP_DIR only). It might lighten up the tests. Might be applicable elsewhere.

ccarouge · 2023-10-06T01:49:26Z

tests/test_comparison.py

+        comparison_task.run()
+        assert f"nccmp -df {file_a} {file_b}" in mock_subprocess_handler.commands
+
+    def test_default_standard_output(self, comparison_task, files):


I wonder if we could instead use the test parameterization to write one output test that runs on all output cases. But this might wait for after the reorganisation?

It should be possible to use parameterisation for testing different verbosity levels. I'll look into it

SeanBryan51 · 2023-10-06T02:12:03Z

do we have to do all the tests in the same PR? It might make it very very hard to review. Could we do one module per PR or only a couple at a time?

I think I prefer we do the merge into main as a single PR. But we can treat this branch as 'main' for this issue and create other branches that implement small changes and have those merged into this branch?

ccarouge · 2023-10-06T02:35:17Z

But we can treat this branch as 'main' for this issue and create other branches that implement small changes and have those merged into this branch?

Yes, that would be better!

bschroeter · 2023-10-08T22:58:46Z

tests/conftest.py

+
+
+@pytest.fixture
+def config():


So I am going to approve this section, but bearing in mind that some of the changes I've made to the configuration validation system (which now has the bonus feature of installing a data directory [which contains tests]) have simplified this process significantly.

I'll have a fun merge on my hands after this one...

SeanBryan51 · 2023-10-10T03:48:25Z

@ccarouge I've rebased this branch off main so that we have the recently merged changes (e.g. linting with ruff). I've added the changes you requested for using pytest's parametrize() and usage of mock_cwd.

ccarouge · 2023-10-11T04:27:41Z

@SeanBryan51 It seems you asked my review again on this meta-PR. Is that correct?

SeanBryan51 · 2023-10-11T04:35:18Z

@ccarouge Yes, just checking I've addressed your comments in: 19eff9e

Unit tests have an excessive amount of repeated test setup code, making unit tests harder to maintain and read. Using pytest fixtures to setup each test would remove needless code repetition and also prevents the possibility of state leaking into each test case. Organise the tests so that for each function `func`, we have a test class `TestFunc` with each method of `TestFunc` containing a single test (success or failure case). Use pytest's `parametrize()` feature for testing different levels of verbosity. Remove usage of `mock_cwd` fixture unless it is required for a test to pass. Fixes #163

SeanBryan51 linked an issue Oct 6, 2023 that may be closed by this pull request

Tidy up organisation of unit tests #163

Closed

SeanBryan51 requested review from bschroeter and ccarouge October 6, 2023 01:06

SeanBryan51 marked this pull request as ready for review October 6, 2023 01:06

ccarouge reviewed Oct 6, 2023

View reviewed changes

bschroeter approved these changes Oct 8, 2023

View reviewed changes

bschroeter mentioned this pull request Oct 8, 2023

config.yaml input validation #143

Closed

SeanBryan51 mentioned this pull request Oct 9, 2023

Refactor work_dir creation #171

Merged

SeanBryan51 force-pushed the 163-tidy-up-organisation-of-unit-tests branch from 30f4ce3 to 19eff9e Compare October 10, 2023 03:43

SeanBryan51 requested a review from ccarouge October 10, 2023 03:47

ccarouge approved these changes Oct 11, 2023

View reviewed changes

ccarouge mentioned this pull request Oct 12, 2023

Organise unit tests to use pytest fixtures (implement remaining files) #178

Merged

SeanBryan51 force-pushed the 163-tidy-up-organisation-of-unit-tests branch from c03e814 to eaa440b Compare October 12, 2023 05:42

SeanBryan51 force-pushed the 163-tidy-up-organisation-of-unit-tests branch from 9d88427 to 107cc86 Compare October 12, 2023 23:56

SeanBryan51 merged commit 058f465 into main Oct 13, 2023

SeanBryan51 deleted the 163-tidy-up-organisation-of-unit-tests branch October 13, 2023 00:10

SeanBryan51 mentioned this pull request Oct 13, 2023

Absolute paths in logged standard output #162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Organise unit tests to use pytest fixtures #172

Organise unit tests to use pytest fixtures #172

SeanBryan51 commented Oct 6, 2023 •

edited

Loading

codecov bot commented Oct 6, 2023 •

edited

Loading

SeanBryan51 commented Oct 6, 2023

ccarouge left a comment

ccarouge Oct 6, 2023

ccarouge Oct 6, 2023

SeanBryan51 Oct 6, 2023

SeanBryan51 commented Oct 6, 2023

ccarouge commented Oct 6, 2023

bschroeter Oct 8, 2023

SeanBryan51 commented Oct 10, 2023

ccarouge commented Oct 11, 2023

SeanBryan51 commented Oct 11, 2023

Organise unit tests to use pytest fixtures #172

Organise unit tests to use pytest fixtures #172

Conversation

SeanBryan51 commented Oct 6, 2023 • edited Loading

codecov bot commented Oct 6, 2023 • edited Loading

Codecov Report

SeanBryan51 commented Oct 6, 2023

ccarouge left a comment

Choose a reason for hiding this comment

ccarouge Oct 6, 2023

Choose a reason for hiding this comment

ccarouge Oct 6, 2023

Choose a reason for hiding this comment

SeanBryan51 Oct 6, 2023

Choose a reason for hiding this comment

SeanBryan51 commented Oct 6, 2023

ccarouge commented Oct 6, 2023

bschroeter Oct 8, 2023

Choose a reason for hiding this comment

SeanBryan51 commented Oct 10, 2023

ccarouge commented Oct 11, 2023

SeanBryan51 commented Oct 11, 2023

SeanBryan51 commented Oct 6, 2023 •

edited

Loading

codecov bot commented Oct 6, 2023 •

edited

Loading