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

add ctest as test driver #3446

Merged

Conversation

felixschurk
Copy link
Collaborator

@felixschurk felixschurk commented May 7, 2024

Basically one can now invoke the whole test suite with running

cmake -S . -B build
cmake --build build --target build_tests
ctest --test-dir build/test 

and one does has a quite decent output of the actual test runs.

Parallel running -j 8, running specific test cases -R cpp or -R variant, having minimal progress bar --progress as well as repeating tests until failure --repeat-until-failure 100 is also possible with CTest.

* add `enable_testing()` in top level CMakeLists to discover tests
* add `add_test` for both unit tests as well as integration tests with
  prefixing the tests with the corresponding test type
* remove custom targets for tests
* remove uneeded warning from https://gitlab.kitware.com/cmake/cmake/-/issues/16062
* pass rerun_failed option to first invocation
* second invocation would be in serial
Copy link
Collaborator

@ryneeverett ryneeverett left a comment

Choose a reason for hiding this comment

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

By switching from CMD to RUN for the test invocation in the docker images, the tests are now run when the images are built and nothing happens when they are run.

@felixschurk
Copy link
Collaborator Author

Yes that is true, I realized that yesterday but could not fix it on the spot. My knowledge with Docker is limited.

Would it maybe make sense to move the test invocation out into the test.yaml file?
I mean updating this line

run: docker-compose run test-$CONTAINER

to

run: docker-compose run test-$CONTAINER ctest --test-dir build/test -j 8 --rerun-failed --output-on-failure

This way we would not have duplicated code in all the docker files, however it maybe would be a bit less/or maybe even more clear how we do run the tests.

@ryneeverett
Copy link
Collaborator

In my opinion reducing the duplication is beneficial and passing the command into the docker run invocation is equally clear to having a CMD in the image.

@felixschurk felixschurk force-pushed the ctest-as-test-runner branch 4 times, most recently from 7c5a89d to b5b025e Compare May 8, 2024 17:44
@felixschurk
Copy link
Collaborator Author

I am a bit sorry for all the failing CI jobs and commit updates, however locally testing it with one image/container it does run. So I am guessing there is a problem with escaping etc. in the tests.yaml file.

I had a similar issue already when setting up another CI. (locally working but in GH Actions not).

@ryneeverett
Copy link
Collaborator

I don't think --entrypoint is what you want. It looks like you were close with 1888816. CI failed with "No such service: test-fedora40 ctest". Maybe you just need some quotes so it splits the arguments correctly?

        run: docker-compose run 'test-${{ env.CONTAINER }}' 'ctest --test-dir build/test -j 8 --output-on-failure --rerun-failed'

(Not 100% sure if it should be single or double quotes but I assume the template variables are substituted prior to evaluation and thus don't care.)

@felixschurk felixschurk force-pushed the ctest-as-test-runner branch 3 times, most recently from c1b7e98 to d18ec43 Compare May 8, 2024 19:47
@felixschurk
Copy link
Collaborator Author

Hm, yes definetely the splitting fixed one problem.
I then detected some problem with a whitespace (which as somehow different encoded).

Now it does not find some file/directory. Let's see what the problem ist.

@felixschurk felixschurk force-pushed the ctest-as-test-runner branch 2 times, most recently from aa9b176 to 6839423 Compare May 8, 2024 20:10
Copy link
Collaborator

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

This is great! I love the more succinct output. A few notes from my experience that don't need to be fixed before this is landed:

  • The ctest invocation, and similarly make -Cbuild test, don't seem to rebuild tests if their source files have changed, which can lead to surprises when a test run keeps failing after the test has been fixed. Maybe cmake --build-and-test helps with this?
  • The tests take a loong time to run. ctest takes a -jN option just like make, which might be a useful thing to recommend (although it did cause ids.test.py to fail for me!)
  • Having a documented way to run a single test will be useful, too.

One thing that does need to be addressed here, is updating the docs on how to run the tests (doc/devel/contrib/development.md).

if(POLICY CMP0037 AND ${CMAKE_VERSION} VERSION_LESS "3.11.0")
cmake_policy(SET CMP0037 OLD)
endif()

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for getting rid of this!

@felixschurk felixschurk force-pushed the ctest-as-test-runner branch 3 times, most recently from 914f1e4 to e6e9dc3 Compare May 9, 2024 04:14
@felixschurk
Copy link
Collaborator Author

In my opinion reducing the duplication is beneficial and passing the command into the docker run invocation is equally clear to having a CMD in the image.

I am sorry, I was not able to work that out. Maybe it makes sense to have this as a new issue (should be an easy one) when someone knows GitHub Actions and Docker.

@felixschurk
Copy link
Collaborator Author

Currently, missing is the update of the test/README.

I also realized now that basically the --repeat or respectively --repeat-until-failure options from ctest do work as a stress test, which we removed in #3447, therefore the functionality is still available, however now a bit more unified.

@felixschurk felixschurk marked this pull request as ready for review May 9, 2024 05:35
Copy link
Collaborator

@djmitche djmitche 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 - let's merge it!

doc/devel/contrib/development.md Outdated Show resolved Hide resolved
```
$ ./problems

Further it is adviced to add the `--output-on-failure` option to `ctest`, to recieve a verbose output if a test is failing as well as the `--rerun-failed` flag, to invoke in subsequent runs only the failed ones.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

@djmitche djmitche enabled auto-merge (squash) May 10, 2024 01:15
@djmitche djmitche merged commit 82e0d53 into GothenburgBitFactory:develop May 10, 2024
12 checks passed
@felixschurk felixschurk deleted the ctest-as-test-runner branch May 10, 2024 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants