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

Fix tests to comply with latest pytest and pytest-split #846

Merged
merged 5 commits into from
Jan 31, 2024

Conversation

dpanici
Copy link
Collaborator

@dpanici dpanici commented Jan 31, 2024

The way pytest catches exceptions and multiple warnings changed, see from the changelog:

#9288: warns() now re-emits unmatched warnings when the context closes – previously it would consume all warnings, hiding those that were not matched by the function.

which is the reason why the fft warning test fails with pytest==8.0.0 now (it returned multiple warnings, and now warns() catches only the desired warning thrown and emits any uncatched ones), and I think this is also the reason the ascii_io test fails now (the file was never closed and a ResourceWarning was raised, but was ignored, and now it gets emitted and so causes the test fail).

My local pytest version is 7.3.1 while the CI uses 8.0.0. If I upgrade my local pytest to 8.0.0 then my tests fail in the same way the CI does. The CI before last week would install 7.4.4 pytest, not 8.0.0, because pytest-split versions <0.8.2 did not support pytest==8.0.0. However on Jan. 29 pytest-split==0.8.2 was release and that one did support the pytest==8.0.0 so our CI started installing that one, hence the errors.

Locally we should all update our pytest-split to 0.8.2and our pytest to 8.0.0 (you need to update both, 8.0.0 pytest is not supported by any pytest-split versions older than 0.8.2) That way the way the CI tests behave and our local should match.

This PR fixes the tests that were a little sloppy anyways, and then restricts the pytest in our requirements to be 8.0.0 to avoid these issues of the CI tests not matching our local tests,.

(this is one way to handle this, we could alternatively restrict the pytest and pytest split versions to be what they used to be, but I am in favor of just bumping up these dependency versions so we use newer versions, these are just for testing anyways so not that restrictive, and these updates to pytest make sense to me as far as improving our tests)

Changes

  • Restrict pytest version to be ~=8.0.0 and pytest-split==0.8.2
  • add missing file close to ascii io, and make user warning in test more specific, to fix ResourceWarning that caused test to fail
  • split the test_fft_warnings test where the grid and basis NFP do not match to test for both warnings that are thrown (both UserWarning but one from the grid and basis not matching and the other for the nodes not completing 1 full field period), as in the past the test only caught the nodes not completing a full period warning and ignored the other warning, but now it throws errors as it sees the grid and basis warning which was previously discarded)

Copy link
Contributor

github-actions bot commented Jan 31, 2024

|             benchmark_name             |         dt(%)          |         dt(s)          |        t_new(s)        |        t_old(s)        | 
| -------------------------------------- | ---------------------- | ---------------------- | ---------------------- | ---------------------- |
 test_build_transform_fft_lowres         |     +0.50 +/- 1.41     | +6.27e-05 +/- 1.77e-04 |  1.26e-02 +/- 1.3e-04  |  1.25e-02 +/- 1.2e-04  |
 test_build_transform_fft_midres         |     +0.43 +/- 1.07     | +3.95e-04 +/- 9.75e-04 |  9.13e-02 +/- 6.8e-04  |  9.09e-02 +/- 7.0e-04  |
 test_build_transform_fft_highres        |     +1.33 +/- 0.80     | +6.09e-03 +/- 3.65e-03 |  4.64e-01 +/- 2.8e-03  |  4.58e-01 +/- 2.3e-03  |
 test_equilibrium_init_lowres            |     -0.42 +/- 1.23     | -3.35e-03 +/- 9.89e-03 |  8.02e-01 +/- 5.4e-03  |  8.05e-01 +/- 8.3e-03  |
 test_equilibrium_init_medres            |     +0.22 +/- 1.21     | +3.15e-03 +/- 1.71e-02 |  1.42e+00 +/- 1.2e-02  |  1.42e+00 +/- 1.2e-02  |
 test_equilibrium_init_highres           |     +0.01 +/- 0.94     | +4.02e-04 +/- 3.96e-02 |  4.21e+00 +/- 3.0e-02  |  4.21e+00 +/- 2.6e-02  |
 test_objective_compile_dshape_current   |     +0.09 +/- 8.68     | +4.01e-03 +/- 3.81e-01 |  4.39e+00 +/- 2.7e-01  |  4.39e+00 +/- 2.6e-01  |
 test_objective_compile_atf              |     +1.23 +/- 6.15     | +1.13e-01 +/- 5.68e-01 |  9.34e+00 +/- 3.5e-01  |  9.23e+00 +/- 4.5e-01  |
 test_objective_compute_dshape_current   |     +3.63 +/- 4.19     | +7.68e-05 +/- 8.88e-05 |  2.19e-03 +/- 8.7e-05  |  2.12e-03 +/- 1.9e-05  |
 test_objective_compute_atf              |     -2.52 +/- 3.70     | -2.00e-04 +/- 2.94e-04 |  7.75e-03 +/- 1.2e-04  |  7.95e-03 +/- 2.7e-04  |
 test_objective_jac_dshape_current       |     -2.65 +/- 12.25    | -1.27e-03 +/- 5.86e-03 |  4.65e-02 +/- 2.8e-03  |  4.78e-02 +/- 5.1e-03  |
 test_objective_jac_atf                  |     +4.05 +/- 5.01     | +9.07e-02 +/- 1.12e-01 |  2.33e+00 +/- 1.1e-01  |  2.24e+00 +/- 3.5e-02  |
 test_perturb_1                          |     +3.30 +/- 14.47    | +2.84e-01 +/- 1.25e+00 |  8.92e+00 +/- 9.4e-01  |  8.63e+00 +/- 8.3e-01  |
 test_perturb_2                          |     -4.90 +/- 5.39     | -7.67e-01 +/- 8.43e-01 |  1.49e+01 +/- 4.7e-01  |  1.57e+01 +/- 7.0e-01  |

@dpanici dpanici changed the title add missing file close to ascii io, and make user warning in test mor… Fix tests to comply with latest pytest and pytest-split Jan 31, 2024
f0uriest
f0uriest previously approved these changes Jan 31, 2024
pytest >= 5.0.0
pytest ~= 8.0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need it >5.0.0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ~= means it must be >=8.0.0, <=8.1.0

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (546cac3) 94.90% compared to head (c5dae6f) 94.90%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #846   +/-   ##
=======================================
  Coverage   94.90%   94.90%           
=======================================
  Files          80       80           
  Lines       19600    19601    +1     
=======================================
+ Hits        18602    18603    +1     
  Misses        998      998           
Files Coverage Δ
desc/io/ascii_io.py 91.08% <100.00%> (+0.08%) ⬆️

@dpanici
Copy link
Collaborator Author

dpanici commented Jan 31, 2024

@daniel-dudt why did you add a commit to this? EDIT: I see to fix a test

@dpanici dpanici merged commit 786434d into master Jan 31, 2024
17 checks passed
@dpanici dpanici deleted the dp/hotfix-io-file-close branch January 31, 2024 22:29
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.

None yet

4 participants