Skip to content

cmake: Add UniValue tests to the ctest suit#73

Merged
hebasto merged 2 commits into
cmake-stagingfrom
240109-cmake-AV
Feb 26, 2024
Merged

cmake: Add UniValue tests to the ctest suit#73
hebasto merged 2 commits into
cmake-stagingfrom
240109-cmake-AV

Conversation

@hebasto
Copy link
Copy Markdown
Owner

@hebasto hebasto commented Jan 9, 2024

As it was pointed out by TheCharlatan and fanquake during the recent offline meeting, since bitcoin#25369, the Univalue unit tests must be included to the ctest suit.

@hebasto
Copy link
Copy Markdown
Owner Author

hebasto commented Jan 16, 2024

Rebased.

@hebasto hebasto added the enhancement New feature or request label Jan 27, 2024
@hebasto hebasto force-pushed the 240109-cmake-AV branch 2 times, most recently from 5461cf2 to 7ec0697 Compare February 6, 2024 13:23
@hebasto hebasto marked this pull request as ready for review February 9, 2024 18:42
@hebasto
Copy link
Copy Markdown
Owner Author

hebasto commented Feb 9, 2024

Rebased and undrafted.

Copy link
Copy Markdown

@theuni theuni left a comment

Choose a reason for hiding this comment

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

ACK d554706. Looks good, built and ran locally. Tried changing JSON_TEST_SRC to a nonsense value and things broke as expected.

Comment thread cmake/tests.cmake
add_all_test_targets()
endif()

if(TARGET unitester)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Out of curiosity, why are these defined here rather than inline with the build rules?

Specifically here, why not do the add_test in src/univalue/CMakeLists.txt ?

I don't have any objection, I just keep forgetting to ask.

Copy link
Copy Markdown
Owner Author

@hebasto hebasto Feb 24, 2024

Choose a reason for hiding this comment

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

The ctest executable looks for tests in the current directory, which can be specified explicitly using the --test-dir option in CTest >=3.20. When using the add_test command in CMake, a new test is placed into the build tree with the same relative path as the location of the file, which executes the command, in the source tree. This behavior is consistent with how the add_executable and add_library commands work.

Therefore, the purpose of the cmake/tests.cmake file is to collect all add_test commands, ensuring that all tests are discoverable by ctest within a single directory.

EDIT. Please ignore this comment.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@theuni

My previous comment is not correct. It was based on the buggy behavior that has been fixed in bfe7c4d.

Please see #275.

Copy link
Copy Markdown

@sedited sedited left a comment

Choose a reason for hiding this comment

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

ACK d554706

@hebasto hebasto merged commit 71e01e3 into cmake-staging Feb 26, 2024
Copy link
Copy Markdown

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

post-merge tACK d554706

Verified the UniValue tests were added (before total was 125, now 127)
        Start 126: univalue_test
111/127 Test #126: univalue_test ...............................................................   Passed    0.02 sec
        Start 127: univalue_object_test
112/127 Test #127: univalue_object_test ........................................................   Passed    0.00 sec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants