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 10 commits into
base: develop
Choose a base branch
from
+16 −9
Open

Improve unit tests #2391

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 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
598e23d
Merge branch 'develop' into develop
bigfooted Dec 27, 2024
8382186
Merge branch 'develop' into develop
Cristopher-Morales Jan 2, 2025
c648c5f
Merge branch 'develop' into develop
Cristopher-Morales Jan 11, 2025
b9cf11b
Merge branch 'develop' into develop
Cristopher-Morales Jan 14, 2025
e11d90a
Merge branch 'develop' into develop
Cristopher-Morales Jan 23, 2025
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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, Guus,
All tests that are not unit tests are run after installation/compilation, and are there mainly to make sure that developers do not break existing functionality during development. The tests all represent very small 'real' testcases that are solved, and we compare the output with the known and stored output.
The unit tests are special because they involve testing for correctness of specific c++ functions that we have, so they involve the compilation of a small cpp file that calls the specific function.
So the unit tests have to know where the header files are, the rest of the tests only have to know where the executable is. Hope that helps.

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
Loading