Better test skipping and basic project linting #643
Conversation
|
thanks for this @mwtoews. The codebase could defo benefit from a clean up, I am wondering how tolerant we want to be of failed imports and missing binaries though? Those red 'x's are a pretty strong motivator to fix broken links between packages and dependancies. |
There was a problem hiding this comment.
I think this notebook wrapper is no longer hit -- at some point, we should check and remove,
|
|
||
| def dsi_freyberg(tmp_d,transforms=None,tag=""): | ||
| if not ies_exe_path: | ||
| pytest.skip("missing ies_exe_path") |
There was a problem hiding this comment.
assume we will throw a fail elsewhere in the CI if bins are missing -- might be worth checking this.
|
|
||
| def gpr_compare_invest(): | ||
| if not mou_exe_path: | ||
| pytest.skip("missing mou_exe_path") |
There was a problem hiding this comment.
These "invest" funcs shouldn't be collected by pytest in the first place, so the "skip" probs ineffective.
There was a problem hiding this comment.
I think this is also mostly legacy func testing -- at some point we should check and remove
| # import sys | ||
| # sys.path.insert(0, os.path.join("..", "..", "pypestutils")) | ||
| import pypestutils as ppu | ||
| ppu = pytest.importorskip("pypestutils") |
There was a problem hiding this comment.
we probably don't want to be skipping this test if import fails.
Currently, there is the pyemu geostats interp fallback if ppu is not available (though this may be deprecated in the future if we want to lean more on ppu)
So here, I think we either want to raise if import fails or continue (which would mean that we are testing the pyemu fallback).
|
|
||
|
|
||
| def mf6_freyberg_da_test(tmp_path): | ||
| flopy = pytest.importorskip("flopy") |
There was a problem hiding this comment.
I think for these we can just do a straight import flopy. flopy should be defined as a testing dependency (only) so if you are running these tests you do need flopy so these would be better to raise rather than skip?
| # import sys | ||
| # sys.path.insert(0,os.path.join("..","..","pypestutils")) | ||
| import pypestutils as ppu | ||
| ppu = pytest.importorskip("pypestutils") |
There was a problem hiding this comment.
ditto above, either we should be raising here if ppu absent or (for now) continuing and allowing pyemu geostationary fallback
There was a problem hiding this comment.
generally for the tests in here we rely on flopy (which should be a testing-only dependency for pyemu). So I think ok to raise rather than skip if flopy import fails. For dealing with ppu import fails at the moment (while we still have the pyemu geostat fallback) I think we can continue but in the future we could look to retire the pyemu fallback and require ppu [perhaps we can set up a discussion on this?], in which case raising on ppu import fails would be appropriate.
There was a problem hiding this comment.
same as in pst_from_tests regarding flopy and ppu imports -- maybe raising is preferable to skipping?
3 participants
The primary change in this PR is to do better test skipping if optional dependencies or executables are not available.
It also applies a few basic lint fixes discovered (and sometimes auto-fixed) with
ruff check. So only a small subset of these are addressed in this PR:where after this PR the remaining are:
(And I don't think all of these need to be addressed and can be ignored).