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

cmake: Report skipped tests properly #272

Merged
merged 2 commits into from
Jul 22, 2024
Merged

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Jul 19, 2024

CMake allows to report about skipped tests properly instead of passing dummy test cases. For instance, on Windows:

> ctest --build-config Release -R compilerbug
Test project C:/Users/hebasto/source/repos/bitcoin/build
    Start 26: compilerbug_tests
1/1 Test #26: compilerbug_tests ................***Skipped   0.02 sec

100% tests passed, 0 tests failed out of 1

Total Test time (real) =   0.04 sec

The following tests did not run:
         26 - compilerbug_tests (Skipped)

We have 3 such cases:

  • src/test/compilerbug_tests.cpp
  • src/test/raii_event_tests.cpp
  • src/test/system_tests.cpp

This PR handles the first two cases.

Once bitcoin#30454 is merged, the following lines can be deleted:

// At least one test is required (in case ENABLE_EXTERNAL_SIGNER is not defined).
// Workaround for https://github.com/bitcoin/bitcoin/issues/19128
BOOST_AUTO_TEST_CASE(dummy)
{
BOOST_CHECK(true);
}

@hebasto
Copy link
Owner Author

hebasto commented Jul 20, 2024

cc @maflcko @theuni

@hebasto
Copy link
Owner Author

hebasto commented Jul 20, 2024

From https://github.com/hebasto/bitcoin/actions/runs/10015755552/job/27687669760:

...
        Start  27: compilerbug_tests
 24/133 Test  #27: compilerbug_tests ....................***Skipped   0.02 sec
...

100% tests passed, 0 tests failed out of 133

Total Test time (real) = 252.67 sec

The following tests did not run:
	 27 - compilerbug_tests (Skipped)

Copy link

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -39,6 +39,9 @@ if(TARGET test_bitcoin)
add_test(NAME ${test_suite_name}
Copy link

Choose a reason for hiding this comment

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

Any reason to even support this? Seems fine to just let devs call $build_dir/test_bitcoin -t suite/case manually for now (just like in autotools)?

Copy link
Owner Author

Choose a reason for hiding this comment

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

This allows leveraging parallelism when running ctest -j <number_of_jobs>. Basically, this mirrors the behaviour of make check in the master branch.

@hebasto hebasto merged commit ad99612 into cmake-staging Jul 22, 2024
39 of 40 checks passed
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.

2 participants