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

Switch to CTest #87

Merged
merged 3 commits into from
Aug 30, 2024
Merged

Switch to CTest #87

merged 3 commits into from
Aug 30, 2024

Conversation

jngrad
Copy link
Member

@jngrad jngrad commented Aug 28, 2024

Replace the Makefile-based test driver by the CTest driver. Tests can now run in parallel with make unit_tests -j8.

* use a test driver to run test cases in parallel
* configure test cases with labels and arguments
* only show test case output on test failure
* allow skipped test cases (error code 5)
@jngrad
Copy link
Member Author

jngrad commented Aug 28, 2024

The CTest tool is described in more detail in the the "Mastering CMake" book, chapter Testing With CMake and CTest. The CMake community provides detailed instructions on Using CTest and CDash without CMake. I skipped the CDash part, since I don't have much experience with the dashboard and wasn't sure how it would integrate in GitHub Actions. Most pyMBE and ESPResSo users are already familiar with CTest, and its ASCII-based reporting should be sufficient for our needs.

Long tests are sorted by decreasing runtime. This way, the first time make functional_tests is called, the CTest scheduler runs the slowest tests first. This cuts the samples job runtime from 50 min down to 20 min (logfile 29390808749). This only works the first time, though! For end users who run make functional_tests or make tests multiple times, the test order may not match with the order specified in testsuite/CTestTestfile.cmake after the first run. That's because the CTest scheduler keeps track of test statistics in testsuite/Testing/Temporary, such as runtimes and test failures, and uses this information to improve future runs. For example, failed tests in the last run will be scheduled to start first the next run, so as to give users rapid feedback on whether their latest code changes have fixed the failing tests.

While this PR adds CMake as a dependency, I have no intention of making pyMBE a CMake project. CMake is required by CTest, but anyone can edit the testsuite/CTestTestfile.cmake without the need to be familiar with the CMake syntax. In fact, variables like CMAKE_CURRENT_SOURCE_DIR are not even available, since we don't have a CMake configuration stage. This can come as a surprise to experienced CMake users.

@jngrad jngrad requested a review from pm-blanco August 28, 2024 21:25
Copy link
Collaborator

@pm-blanco pm-blanco left a comment

Choose a reason for hiding this comment

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

Thank you very much for this PR and the useful links @jngrad! I did some testing in my office computer and the new framework for tests seems to work very well and is way faster than before, which will save both on our Github quota and on the time spending debugging code. I also think that it should be clear from your notes how to add new tests with the new framework, but @paobtorres should take a look into this PR since she is the one that is going to develop tests in the near future.

I think that we should note in the requierements that cmake is a dependency only for testing. Since we are working in a virtual environment it should in principle not be a problem to have it installed by default, but just in case it might be a potential issue for some of our users. I added a similar note on tqdm which is actually only needed because we use it in some of our sample scripts

testsuite/henderson_hasselbalch_tests.py Outdated Show resolved Hide resolved
testsuite/henderson_hasselbalch_tests.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
requirements.txt Show resolved Hide resolved
Copy link
Collaborator

@pm-blanco pm-blanco left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for the detailed explanation @jngrad. I took a look into poetry and I agree with you: it is an interesting tool but not what we need at the moment. I do not think that it is worth for now to add another layer of complexity to our library. Thanks for the refactoring of the HH tests, the ut.TestCases sounds like a quite convenient framework. We can try to use it more for our other tests!

@pm-blanco pm-blanco merged commit 22079d5 into pyMBE-dev:main Aug 30, 2024
3 checks passed
@jngrad jngrad deleted the ctest branch August 30, 2024 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants