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

Refactor tests, adding a common module #138

Merged
merged 7 commits into from
Dec 5, 2024

Conversation

mwtoews
Copy link
Contributor

@mwtoews mwtoews commented Dec 1, 2024

This PR simplifies a few ambitions of #108 to:

  • 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 hatch 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
  • Install MODFLOW 6 using https://github.com/modflowpy/install-modflow-action
  • Show path for libmf6 in the pytest report header
  • Move common pytest settings to [tool.pytest.ini_options] in pyproject.toml
  • Fix a Ruff check warning that the top-level linter settings are deprecated
  • Compact white space used with data used in flopy_disu fixture.

Copy link

codecov bot commented Dec 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.33%. Comparing base (75cfe52) to head (de18881).
Report is 1 commits behind head on develop.

@@           Coverage Diff            @@
##           develop     #138   +/-   ##
========================================
  Coverage    87.33%   87.33%           
========================================
  Files            7        7           
  Lines          521      521           
========================================
  Hits           455      455           
  Misses          66       66           

@mwtoews
Copy link
Contributor Author

mwtoews commented Dec 1, 2024

I don't have a mac, so I can't debug the unrelated issue highlighted by this PR:

At line 161 of file ../srcbmi/mf6bmiUtil.f90
Fortran runtime error: Dimension 1 of array 'c_array' has extent 10 instead of 256

The referenced runtime error location is linked here via here.

And the failing test is linked here.

@mjr-deltares
Copy link
Contributor

thanks @mwtoews , I will have a look at that issue and then I think we can put this in

@mjr-deltares
Copy link
Contributor

@mwtoews I have added checks to the MODFLOW 6 autotests to see if the current development version throws the same exception when running the API on macos but it doesn't. Could you try your workflow taking the latest version from the repo?

@mwtoews
Copy link
Contributor Author

mwtoews commented Dec 4, 2024

A change was needed for the test, as SLN_1/IMSLINEAR/ID is no longer a scalar variable with rank 0 with the latest mf6 builds (it is changed to a 1-D array of 89 zeros). For testing, it was changed to TEST_MODEL_DIS/DIS/NROW which will always be a scalar.

Also, the issue with macos does not apply the the mf6 nightly builds. Only the latest release.

@mwtoews
Copy link
Contributor Author

mwtoews commented Dec 5, 2024

I borrowed a mac laptop to investigate, and I can recreate the issue only with mf6.5.0_macarm.zip from https://github.com/MODFLOW-USGS/modflow6/releases/tag/6.5.0 . Whereas mf6.5.0_mac.zip (x64_86) runs fine, without issue. And the modflow6-nightly-build repo is fine with both mac platforms, which means the issue is resolved. So I suspect there was an issue with the 6.5.0 release for arm64.

@mjr-deltares
Copy link
Contributor

mjr-deltares commented Dec 5, 2024

it is changed to a 1-D array of 89 zeros

You are right, the particular ID selected in this case is not an identification number but an integer work array for the linear solver... I think your solution to set it to NROW is safer.

@mjr-deltares
Copy link
Contributor

Btw we are planning to release 6.6.0 any day now, so glad to know that that issue has been resolved.

@mjr-deltares mjr-deltares merged commit ea3360f into Deltares:develop Dec 5, 2024
16 checks passed
@mjr-deltares
Copy link
Contributor

For completeness, this is the fix that prevents the nightly build from showing the same behavior as the 6.5.0 release: MODFLOW-USGS/modflow6#1996

@mwtoews mwtoews deleted the reorg-tests-common branch December 5, 2024 17:01
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.

2 participants