Skip to content

Conversation

@mikemhenry
Copy link
Contributor

This PR unifies how we skip tests.

Checklist

  • Added a news entry

Developers certificate of origin

@mikemhenry mikemhenry requested a review from atravitz October 29, 2025 17:32
@github-actions
Copy link

No API break detected ✅

@codecov
Copy link

codecov bot commented Oct 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.89%. Comparing base (e48d641) to head (b25674e).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1640      +/-   ##
==========================================
- Coverage   95.33%   92.89%   -2.45%     
==========================================
  Files         183      183              
  Lines       15762    15779      +17     
==========================================
- Hits        15027    14658     -369     
- Misses        735     1121     +386     
Flag Coverage Δ
fast-tests 92.89% <100.00%> (?)
slow-tests ?

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@atravitz
Copy link
Contributor

I agree with unification, but I would generally prefer we use a built-in pytest functionality over an openff utils dependency. is there a benefit to openff's implementation here?

@mikemhenry
Copy link
Contributor Author

mikemhenry commented Oct 29, 2025

It started with me thinking that pytest.importskip was deprecated in 8.3, BUT a closer read is that the behavior of how it handles ImportError differently than ModuleNotFoundError

I like how the openff-utils version only skips on a ModuleNotFoundError and instead would let an ImportError bubble up. I also like how the openff-utils version is a decorator where the pytest version should really be used like this:

np = pytest.importskip("numpy")

def test_cos(rad):
    assert np.cos(0) == 1

so we were not really using it correctly (and often our tests don't really have an import of the optinal package so it looks a bit weird to have pytest.importskip("numpy") inside each test we want to skip).

I am happy to switch to using the built in pytest version, it has some nice things like a minversion arg so if we had a test that needs a newer version of a package but we still supported older versions, this method would just work (assuming we had 2 CI legs testing the new and old version).

I am not really worried about the openff-utilities dep since other packages need it as well:

 openff-interchange-base 0.4.8   pyhd8ed1ab_1   openff-utilities >=0.1.5 conda-forge noarch
 openff-toolkit-base     0.17.1  pyhd8ed1ab_0   openff-utilities         conda-forge noarch
 openff-units            0.3.1   pyhd8ed1ab_2   openff-utilities         conda-forge noarch

@atravitz
Copy link
Contributor

Thanks for articulating this @mikemhenry - I think the benefits (especially decorator) of openff utils is worth it here (and probably why openff added it in the first place 😄) i'm fine with this solution!

@mikemhenry mikemhenry merged commit 0a5265f into main Oct 29, 2025
13 checks passed
@mikemhenry mikemhenry deleted the maint/switch_to_skip_if_missing branch October 29, 2025 21:48
@mikemhenry
Copy link
Contributor Author

As a bonus, b/c we switch to this method, the output looks like this now:

$ pytest -v -k perses                                                                                                                                                       
==================================================================================== test session starts =====================================================================================
platform linux -- Python 3.13.9, pytest-8.4.2, pluggy-1.6.0 -- /home/mmh/micromamba/envs/openfe-1.7.0/bin/python3.13
cachedir: .pytest_cache
rootdir: /home/mmh/Projects/openfe
configfile: pyproject.toml
plugins: datadir-1.8.0, anyio-4.11.0, regressions-2.8.3
collected 1513 items / 1506 deselected / 7 selected                                                                                                                                          

openfe/tests/setup/atom_mapping/test_perses_atommapper.py::test_simple SKIPPED (Package perses is required, but was not found.)                                                        [ 14%]
openfe/tests/setup/atom_mapping/test_perses_atommapper.py::test_generator_length SKIPPED (Package perses is required, but was not found.)                                              [ 28%]
openfe/tests/setup/atom_mapping/test_perses_atommapper.py::test_empty_atommappings SKIPPED (Package perses is required, but was not found.)                                            [ 42%]
openfe/tests/setup/atom_mapping/test_perses_atommapper.py::test_dict_round_trip SKIPPED (Package perses is required, but was not found.)                                               [ 57%]
openfe/tests/setup/atom_mapping/test_perses_scorers.py::test_perses_normalization_not_using_positions SKIPPED (Package perses is required, but was not found.)                         [ 71%]
openfe/tests/setup/atom_mapping/test_perses_scorers.py::test_perses_not_implemented_position_using SKIPPED (Package perses is required, but was not found.)                            [ 85%]
openfe/tests/setup/atom_mapping/test_perses_scorers.py::test_perses_regression SKIPPED (Package perses is required, but was not found.)                                                [100%]

instead of

$ pytest -v -k perses                                                                                                                                                       
==================================================================================== test session starts =====================================================================================
platform linux -- Python 3.13.9, pytest-8.4.2, pluggy-1.6.0 -- /home/mmh/micromamba/envs/openfe-1.7.0/bin/python3.13
cachedir: .pytest_cache
rootdir: /home/mmh/Projects/openfe
configfile: pyproject.toml
plugins: datadir-1.8.0, anyio-4.11.0, regressions-2.8.3
collected 1500 items / 1500 deselected / 3 skipped / 0 selected          

@mikemhenry mikemhenry mentioned this pull request Oct 29, 2025
2 tasks
@atravitz atravitz mentioned this pull request Dec 4, 2025
7 tasks
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