Get the test suite building and running again - #3
Open
Fede654 wants to merge 3 commits into
Open
Conversation
`cmake -DSS_TESTS=ON` currently fails to configure. Four small things,
each independent of the others:
- CMAKE_MODULE_PATH is set to ${CMAKE_SOURCE_DIR}/cmake/, but
Doctest.cmake lives in tests/cmake/, so include(Doctest) cannot find
it. Uses CMAKE_CURRENT_SOURCE_DIR instead.
- target_set_warnings() is called but its Warnings.cmake module is not
present in the repository, which is a hard configure error. Wrapped in
if(COMMAND ...) so it is still used if that module is ever added.
- include(CodeCoverage) is likewise not present. Made OPTIONAL: the
suite then builds, and only the 'coverage' target is unavailable.
- Doctest.cmake fetches doctest inside if(ENABLE_DOCTESTS), and that
variable is never set, so doctest is never fetched and the tests fail
to compile on a missing doctest/doctest.h. tests/CMakeLists.txt is
only added to the build when SS_TESTS is ON, so it sets ENABLE_DOCTESTS
itself.
With these the test target configures and builds. A follow-up commit
deals with the test sources themselves.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
mergeechotest.cc and parsearcomandtest.cc predate the current codebase: they call SharedState::extractCommand and SharedState::mergestate and include shared_state_error_code.hh and piped_async_command.hh, none of which exist since the Lua dependency was removed in 99a4249, so they cannot compile against today's sources. debugmesasgetest.cc still compiles but asserts nothing — it prints debug messages at each level — so it is removed as well rather than kept as a test that cannot fail. This also adds enable_testing() to the top-level CMakeLists.txt. Without it CMake writes no root CTestTestfile.cmake, so `ctest` in the build directory reports "No tests were found" regardless of what the tests subdirectory registers; include(CTest) in tests/CMakeLists.txt only covers that subdirectory. In their place, fourteen cases over code that is still current. bleach_tests.cc covers expiry, which is an ordinary function and so convenient to test directly. Entries at exactly the elapsed time are removed rather than kept; survivors are decremented by exactly that amount; five 2 s bleaches leave the same TTL as one 10 s bleach, which matters because the daemon passes real elapsed time to compensate for late timer ticks; a non-positive interval is refused without altering TTLs; an unknown type reports UNKOWN_DATA_TYPE; and an empty registered type is a no-op rather than an error. mStates is protected, so a small subclass supplies known state — the function under test is the real one. state_serialization_tests.cc covers the shapes peers must agree on. The compatibility cases deserialize *literal* payloads rather than round tripping whatever this build emits, since a round trip agrees with itself however the format drifts: an entry and a whole slice in the shape nodes send today must be accepted, and the member names are asserted directly for both StateEntry and DataTypeConf, because renaming one changes the wire format for every peer and the on-disk config for every node. Two cases document behaviour rather than assert correctness: - When an entry cannot be read, deserialization stops there: entries earlier in the map survive and everything after is lost. The serialization context records the failure, but NetworkMessage::toStateSlice() discards that status, so a node silently merges the surviving prefix. If a member is ever added and required on read, a slice from a peer that does not emit it yet has every entry in an incompatible legacy shape, so the first one fails and that peer's neighbour sees an empty view of it rather than a degraded one. - StateEntry is copy-constructible but not copy-assignable, because rapidjson's Document assignment is private. `map[key] = entry` fails to compile with an error that points into <map>, while `map.emplace(key, entry)` works. The suite deliberately stops short of merge() itself, which is a coroutine needing an IOContext and a peer address; covering it well means driving real instances rather than unit testing it. Verified with cmake -DSS_TESTS=ON in Release and Debug: configures, builds, and ctest passes 1/1 (14 doctest cases, 63 assertions). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two reasons CI does not currently exercise the test suite: - The "switch to gcc-10" step fails outright on today's ubuntu-latest images, which no longer ship gcc-10: update-alternatives reports "alternative path /usr/bin/gcc-10 doesn't exist" and the job stops before configuring. The project requires C++20 coroutines, which the runner's default GCC has supported for several releases, so the pin can simply go. - Configure does not pass -DSS_TESTS=ON, and that option defaults to OFF, so the test target is never added and the `ctest` step reports "No tests were found" while the job goes green. With both addressed the existing Test step runs the suite for real. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cmake -DSS_TESTS=ONdoes not currently configure, and CI does notenable it, so
ctestreports "No tests were found" while the job passes.These three commits get the suite building, replace test sources that no
longer match the API, and let CI run them.
Each commit stands alone and can be taken separately.
1. Build wiring — four independent issues, all in
tests/CMakeLists.txt:CMAKE_MODULE_PATHpoints at${CMAKE_SOURCE_DIR}/cmake/, butDoctest.cmakeis intests/cmake/, soinclude(Doctest)fails.target_set_warnings()is called but itsWarnings.cmakeis not in therepository — a hard configure error. Wrapped in
if(COMMAND ...).include(CodeCoverage)is likewise absent. MadeOPTIONAL.Doctest.cmakefetches doctest insideif(ENABLE_DOCTESTS), which isnever set, so doctest is never fetched and compilation fails on a
missing
doctest/doctest.h.2. Test sources —
mergeechotest.ccandparsearcomandtest.cccallSharedState::extractCommandandSharedState::mergestateand includeshared_state_error_code.hhandpiped_async_command.hh, none of whichexist since 99a4249 removed the Lua dependency.
debugmesasgetest.cccompiles but asserts nothing. Replaced with fourteen cases over current
code:
bleach()boundaries (entries at exactly the elapsed time,decrement amounts, repeated vs single bleach, invalid interval, unknown
type, empty type) and serialization of the shapes peers agree on.
The compatibility cases deserialize literal payloads rather than round
tripping what the build emits, so they keep working as a guard if the
format ever drifts, and they assert the member names directly — renaming
one changes the wire format for every peer and the config file for every
node.
This commit also adds
enable_testing()to the top-levelCMakeLists.txt. Without it CMake writes no rootCTestTestfile.cmakeand
ctestfinds nothing however many tests the subdirectory registers.3. CI — the "switch to gcc-10" step fails on current
ubuntu-latestimages, which no longer ship gcc-10, and configure does not pass
-DSS_TESTS=ON. With both addressed the existing Test step runs thesuite for real.
Verified in Release and Debug locally, and on GitHub Actions against this
branch: 1/1 CTest target, 14 doctest cases, 63 assertions.
Not covered:
merge()itself, which is a coroutine needing anIOContextand a peer address — testing it meaningfully means drivingreal instances rather than unit testing it.