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

Improve unit tests #2391

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
+16 −9
Open

Improve unit tests #2391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c2a668e
Update to Meson 0.64.1.
Dec 8, 2024
82abcad
UnitTests: make Meson copy required drg files to build dir
Dec 6, 2024
9e0549e
UnitTests: do not install tests
Dec 6, 2024
faca833
Merge branch 'develop' into develop
Cristopher-Morales Dec 9, 2024
2d31d2b
Merge branch 'develop' into develop
bigfooted Dec 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions UnitTests/Common/containers/CLookupTable_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
TEST_CASE("LUTreader", "[tabulated chemistry]") {
/*--- smaller and trivial lookup table ---*/

CLookUpTable look_up_table("src/SU2/UnitTests/Common/containers/lookuptable.drg", "ProgressVariable", "EnthalpyTot");
CLookUpTable look_up_table("lookuptable.drg", "ProgressVariable", "EnthalpyTot");

/*--- string names of the controlling variables ---*/

Expand Down Expand Up @@ -87,8 +87,7 @@ TEST_CASE("LUTreader", "[tabulated chemistry]") {
TEST_CASE("LUTreader_3D", "[tabulated chemistry]") {
/*--- smaller and trivial lookup table ---*/

CLookUpTable look_up_table("src/SU2/UnitTests/Common/containers/lookuptable_3D.drg", "ProgressVariable",
"EnthalpyTot");
CLookUpTable look_up_table("lookuptable_3D.drg", "ProgressVariable", "EnthalpyTot");

/*--- string names of the controlling variables ---*/

Expand Down
16 changes: 12 additions & 4 deletions UnitTests/meson.build
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
fs = import('fs')

# Add any new test files here
# -------------------------------------------------------------------------
# Begin unit test listings
Expand Down Expand Up @@ -29,23 +31,30 @@ su2_cfd_tests_dd = files(['Common/simple_directdiff_test.cpp'])

if get_option('enable-tests')
if get_option('enable-normal')
unit_test_deps = [
fs.copyfile('Common/containers/lookuptable.drg', 'lookuptable.drg'),
fs.copyfile('Common/containers/lookuptable_3D.drg', 'lookuptable_3D.drg'),
]
unit_test_files = su2_cfd_tests + files(['test_driver.cpp'])
test_driver = executable(
'test_driver',
unit_test_files,
install : true,
dependencies : [su2_cfd_dep, common_dep, su2_deps, catch2_dep],
cpp_args: ['-fPIC', default_warning_flags, su2_cpp_args]
)
test('Catch2 test driver', test_driver)
test(
'Catch2 test driver',
test_driver,
depends: unit_test_deps,
workdir: meson.current_build_dir(),
)
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,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! I'll have a look!

Copy link
Author

Choose a reason for hiding this comment

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

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.)

Copy link
Author

Choose a reason for hiding this comment

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

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:

  1. Do nothing: the documentation should be changed to reflect that the unit tests will generally not work, unless run under very specific circumstances.
  2. 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.
  3. Remove hardcoded paths, install unit tests, at test time go to correct work dir and run unit tests. Documentation needs an update too.
  4. 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?

Copy link
Author

Choose a reason for hiding this comment

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

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?

dependencies : [su2_cfd_dep_ad, commonAD_dep, su2_deps, codi_dep, catch2_dep],
cpp_args: ['-fPIC', default_warning_flags, su2_cpp_args, codi_rev_args]
)
Expand All @@ -57,7 +66,6 @@ if get_option('enable-tests')
test_driver_DD = executable(
'test_driver_DD',
unit_test_files_dd,
install : true,
dependencies : [su2_cfd_dep_dd, commonDD_dep, su2_deps, codi_dep, catch2_dep],
cpp_args: ['-fPIC', default_warning_flags, su2_cpp_args, codi_for_args]
)
Expand Down
2 changes: 1 addition & 1 deletion externals/meson
Open in desktop
Submodule meson updated 1389 files
2 changes: 1 addition & 1 deletion meson_scripts/init.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def init_submodules(
github_repo_medi = "https://github.com/SciCompKL/MeDiPack"
sha_version_opdi = "a6b9655c240af2a35454a61727e5bbbbaa3a425f"
github_repo_opdi = "https://github.com/SciCompKL/OpDiLib"
sha_version_meson = "41c650a040d50e0912d268af7a903a9ce1456dfa"
sha_version_meson = "e000aa11373298c6c07e264d4436b5075210bd11"
github_repo_meson = "https://github.com/mesonbuild/meson"
sha_version_ninja = "52649de2c56b63f42bc59513d51286531c595b44"
github_repo_ninja = "https://github.com/ninja-build/ninja"
Expand Down