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.

Sign up for GitHub

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

Jump to bottom

Test with modflow6 release and nightly, add test.common module #108

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Test with modflow6 release and nightly, add test.common module #108

wants to merge 2 commits into from

Conversation

mwtoews
Copy link
Contributor

@mwtoews mwtoews commented Jun 18, 2023 •
edited

There are a number of changes related to tests:

  • Add tests/common.py for common test functions that are not fixtures (these are kept at conftest.py). Note that tests is now a module, but it is not included by setuptools for installation/packaging.
  • Move logic that determines libmf6 for tests from conftest.modflow_lib_path to common.libmf6_path, based on:
    1. An environment variable LIBMF6 for the direct path to the .so/.dll/.dylib file
    2. An environment variable MODFLOW_BIN_PATH to the directory with both mf6 and libmf6 (the suffix is determined by platform); this is the default path used by modflowpy/install-modflow-action
    3. Evaluated on the system PATH, assuming that libmf6 is in the same directory as mf6
  • Show path for libmf6 in the pytest report header
  • Modify CI tests to install from the modflow6 release repo, run tests
  • Re-install modflow6 from the nightly build repo, an re-run tests, amending coverage reports
  • Parameterize modflow nightly build repo to test matrix
  • Change pytest invocation by removing s option that disable all capturing. This change is done since the capturing by pytest seems to be usually adequate to debug issues (perhaps this wasn't the case before?)

The other key change is to only run test_err_cvg_failure for libmf6 versions before modflow-6.4.2. (I'll admit that I don't fully understand this change, but this seems to be a deliberate change).

Closes #106

@mwtoews mwtoews force-pushed the add-nightly-skip-test_err_cvg_failure branch 4 times, most recently from 5dd6c1d to eb4089a Compare June 18, 2023 11:09
@codecov
Copy link

codecov bot commented Jun 18, 2023 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.52%. Comparing base (5d7c290) to head (ed2c476).
Report is 6 commits behind head on develop.

@@           Coverage Diff            @@
##           develop     #108   +/-   ##
========================================
  Coverage    87.52%   87.52%           
========================================
  Files            7        7           
  Lines          521      521           
========================================
  Hits           456      456           
  Misses          65       65

@mwtoews mwtoews mentioned this pull request Jun 18, 2023
Draft
Hofer-Julian
Hofer-Julian reviewed Jun 19, 2023
View reviewed changes
Copy link
Contributor

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

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

Thank you, @mwtoews!

.github/workflows/CI.yml Outdated Show resolved Hide resolved
.github/workflows/CI.yml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
tests/common.py Outdated Show resolved Hide resolved
tests/common.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/test_mf6_dis_errors.py Outdated Show resolved Hide resolved
mwtoews
mwtoews commented Jun 23, 2023
View reviewed changes
pyproject.toml Outdated
Comment on lines 36 to 37
[tool.setuptools.packages.find]
include = ["xmipy", "xmipy.*"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is re-added, otherwise "tests" would be added to the .whl package, since it is now a findable module.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks for fixing that.
I removed it since with packages.find submodules were not found.
I guess, I should have added "packagename.*" to fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This now revised with tool.hatch.build.targets.wheel.packages

@mwtoews
Copy link
Contributor Author

mwtoews commented Jun 23, 2023

Current test failures are expected due to MODFLOW-USGS/modflow6#1269 which may hopefully be resolved soon.

@mwtoews mwtoews mentioned this pull request Jul 4, 2023
Closed
@mwtoews mwtoews force-pushed the add-nightly-skip-test_err_cvg_failure branch from 8d3eb94 to 9424342 Compare February 20, 2024 22:54
@mwtoews mwtoews force-pushed the add-nightly-skip-test_err_cvg_failure branch from 9424342 to 9b3b548 Compare February 20, 2024 23:01
@mwtoews mwtoews changed the title Test with modflow6 release and nightly (skip test_err_cvg_failure) Test with modflow6 release and nightly, add test.common module Feb 20, 2024
Hofer-Julian
Hofer-Julian reviewed Feb 21, 2024
View reviewed changes
Copy link
Contributor

@Hofer-Julian Hofer-Julian left a comment

Choose a reason for hiding this comment

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

Two small comments, apart from that looks good to me! :)

.github/workflows/CI.yml
Comment on lines +37 to +39
- name: Run tests with MODFLOW 6 release
run: |
pytest -vs --cov=xmipy --cov-report=xml tests/
pytest -v --cov=xmipy --cov-report=xml
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove the -s?

MODFLOW 6 still crashes for most errors, which means there will be zero output in the CI when tests fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running pytest -s throws way too much to the console, whereas the default will enable pytest to capture the output, showing more "normal" pytests outputs. I'll test locally to see if MF6 crashes are adequately captured with/without the flag...

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know if you got reasonable results.

You could try e.g. an invalid configuration option.

tests/common.py Outdated Show resolved Hide resolved
@mwtoews @Hofer-Julian
Use walrus op in tests/common.py
ed2c476 
Co-authored-by: Hofer-Julian <30049909+Hofer-Julian@users.noreply.github.com>
@mjr-deltares
Copy link
Contributor

I am considering closing this, any objections?

@mjr-deltares mjr-deltares reopened this Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hofer-Julian Hofer-Julian Hofer-Julian left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test_err_cvg_failure does not raise XMIError with modflow-6.5.0-dev
3 participants
@mwtoews @mjr-deltares @Hofer-Julian