Improve unit tests #2391
Improve unit tests #2391
Conversation
Meson 0.64.0 introduces fs.copyfile(). We use the latest patch level of 0.64, which 0.64.1.
| endif | ||
|
|
||
| if get_option('enable-autodiff') | ||
| unit_test_files_ad = su2_cfd_tests_ad + files(['test_driver.cpp']) | ||
| test_driver_AD = executable( | ||
| 'test_driver_AD', | ||
| unit_test_files_ad, | ||
| install : true, |
There was a problem hiding this comment.
We would need to change the regression workflow to find the unit test binaries somewhere else. It's simpler if you add a meson option to skip installing them by default, and then we can install them in the regression test script.
There was a problem hiding this comment.
But... The documentation indicates the unit tests should be run from within the build dir (https://su2code.github.io/docs_v7/Running-Unit-Tests/). Shouldn't the regressions test script do the same, then?
(I'm not an SU2 pro... Maybe the regression test script tests for more and/or other things, which would invalidate the above.)
There was a problem hiding this comment.
I see, there are multiple tests being run:
- UnitTests/test_driver{,_AD,_DD}
- TestCases/...
The former currently relies on hardcoded paths in the source tree, which typically only work in the build container and test container. The hardcoded paths break the unit tests in any other situation.
My patches remove the hardcoded paths, and instead tell Meson to move the files to the build tree. This makes the unit tests work as documented.
Of course, the files can be installed as well. The test_driver then needs to be run from the right dir, or needs to have correct paths hardcoded into it. In the latter case, the documentation needs an update: only run the tests after installing.
So, there are three options:
- Do nothing: the documentation should be changed to reflect that the unit tests will generally not work, unless run under very specific circumstances.
- Remove hardcoded paths, do not install unit tests, run unit tests at build time. This is how it is documented, and how most other software does it.
- Remove hardcoded paths, install unit tests, at test time go to correct work dir and run unit tests. Documentation needs an update too.
- Have build system insert hardcoded paths to $prefix, install unit tests, run unit tests at test time. Documentation needs an update too.
3 and 4 would break things like EasyBuild, that assume tests are run before installation, and require an update to the documentation.
2 seems like the right option.
What do you think?
There was a problem hiding this comment.
Option 2 would require a change to the compileSU2.sh script. I doubt that that will be reflected anytime soon, which results in the tests failing until the container is updated.
@pcarruscag I think options 3 above is what you suggested, but would still either break the documented way to run unit tests (ninja -C builddir test), or require one to specify a workdir in regression.yml, which seems impossible.
What is the intention of tests? Should they be run before installing, to ensure one doesn't install a broken binary, or should they be run after installing?
Proposed Changes
fs.copyfile, which is needed for copying files for unit tests (see next point). The latest patch level within this minor version is 0.64.1.Related Work
PR Checklist
pre-commit run --allto format old commits.Githooks man page says that the pre-commit hook must have run, but I don't know how to verify that it did.