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

CMake maintenance #4468

Merged
merged 7 commits into from
Mar 16, 2022
Merged

CMake maintenance #4468

merged 7 commits into from
Mar 16, 2022

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Mar 9, 2022

Description of changes:

  • rewrite CMake logic for Python tests
    • checkpoint tests: convert the body of the list loop into a function checkpoint_test() and call it once for each combination of features (improves readability and separation of concerns)
    • tests called multiple times with a different number of MPI ranks no longer require creating a new copy of the original file, instead a different CTest name is used
    • Python tests that used to depend on CMake variable substitution are now configured with a class factory; the CMake variable is passed as a command-line argument to the test file and the corresponding test class is generated on-the-fly
  • improve code coverage
    • checkpoint tests coverage data is no longer discarded at random, instead it is accumulated over all feature combinations

jngrad added 6 commits March 7, 2022 19:42
@jngrad
CMake: Don't rename test files when possible
e805401 
When a test case doesn't need CMake variable substitution during
configuration, there is no need to rename it. Renaming test files
hinders Coverage.py's ability to map test files in the build
directory to the corresponding test files in the root directory.
Use FILE_SUFFIX to change the filepath during configuration, and
SUFFIX to change the test name in CTest.
@jngrad
CMake: Remove FILE_SUFFIX
a9c607b 
The checkpoint and ek_eof_one_species tests no longer depend on
CMake variables substitution. Instead, the test parameters are
passed as arguments to the test file and a class factory generates
the test class and parses the test parameters.
@jngrad
CMake: Configure python test files with COPYONLY
adcd8e1
@jngrad
testsuite: Move utility functions to client code
33f750c 
Move utility functions to test cases that use them, if they are used
only once.
@jngrad
CMake: Cleanup python test dependencies
41d632a 
Mark shared python files as explicit dependencies of client tests
instead of passing them to `add_custom_target()`.
@jngrad
CMake: Simplify tests LABELS
7d8752b
@jngrad jngrad requested review from RudolfWeeber and reinaual March 9, 2022 12:59
@pm-blanco
Copy link
Contributor

Now that I see this PR, I noticed that it exists one instruction of cmake make test whose utility is unclear to me. Its wording seems confusing to me compared to make check, which is the one that I use to check that all tests run properly. When I run make test, it runs 379 tests, some of them failing on my computer. However, the continuous integration of github passes in my PRs so I guess that this command is somehow deprecated.

@jngrad
Copy link
Member Author

jngrad commented Mar 11, 2022

Now that I see this PR, I noticed that it exists one instruction of cmake make test whose utility is unclear to me. Its wording seems confusing to me compared to make check, which is the one that I use to check that all tests run properly. When I run make test, it runs 379 tests, some of them failing on my computer. However, the continuous integration of github passes in my PRs so I guess that this command is somehow deprecated.

make test is indeed deprecated. It is a built-in feature of CMake, se we cannot remove it. It doesn't support file dependencies, therefore several tests will fail in a clean build folder where make check was never executed before. More details in #4370.

It also ignores the CMake variable CTEST_ARGS, which is why make test runs the tests one after the other, while make check runs them in parallel batches.

@pm-blanco
Copy link
Contributor

Now that I see this PR, I noticed that it exists one instruction of cmake make test whose utility is unclear to me. Its wording seems confusing to me compared to make check, which is the one that I use to check that all tests run properly. When I run make test, it runs 379 tests, some of them failing on my computer. However, the continuous integration of github passes in my PRs so I guess that this command is somehow deprecated.

make test is indeed deprecated. It is a built-in feature of CMake, se we cannot remove it. It doesn't support file dependencies, therefore several tests will fail in a clean build folder where make check was never executed before. More details in #4370.

It also ignores the CMake variable CTEST_ARGS, which is why make test runs the tests one after the other, while make check runs them in parallel batches.

@jngrad thank you for the explanation!

@jngrad
Copy link
Member Author

jngrad commented Mar 15, 2022

This PR is needed for the upcoming electrostatics/magnetostatics refactoring PR.

reinaual
reinaual previously approved these changes Mar 16, 2022
View reviewed changes
@jngrad jngrad added the automerge Merge with kodiak label Mar 16, 2022
@kodiakhq kodiakhq bot merged commit 240af62 into espressomd:python Mar 16, 2022
@jngrad jngrad deleted the cmake_testsuite_refactor branch March 16, 2022 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reinaual reinaual reinaual approved these changes

@RudolfWeeber RudolfWeeber Awaiting requested review from RudolfWeeber

Assignees
No one assigned
Labels
automerge Merge with kodiak CMake Python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jngrad @pm-blanco @reinaual