Skip to content

Isolate tests and add sanitizer CI coverage - #128

Merged
derekste merged 4 commits into
mainfrom
dev/test-tooling-hygiene
Oct 7, 2026
Merged

derekste merged 4 commits into
mainfrom
dev/test-tooling-hygiene

Conversation

@derekste

@derekste derekste commented Oct 1, 2026

Copy link
Copy Markdown
Member

Older GoogleTest cases shared the TEST key namespace, and redis-start.sh could silently reuse an existing Redis server. This follow-up gives each case its own namespace, rejects occupied fixture endpoints, runs the normal suite in parallel, and adds ASan+UBSan and TSan jobs against the pinned private Redis fixture.

For #125, Dependabot now uses docker-compose, excludes GoogleTest/hiredis from automatic submodule updates, and documents deliberate upstream release checks. All checkout steps disable persisted credentials. The parent stack already honors the requested C++ standard, makes callback wait flags atomic, and enables checks on stacked PRs. This PR targets #111 so the sanitizer jobs cover those library fixes.

Validation: all 107 standalone cases passed on Linux/Redis 7.4.2 with ctest -j8; all 107 also passed under GCC 15.2 TSan with halt_on_error=1 and ASLR disabled. The Cluster case uses its separate CI fixture. The macOS C++20 build and parallel legacy cases pass, and the occupied-port preflight exits with an error. ASan+UBSan will also run on this PR.

@bigsamich ready for review against the revised recovery stack.

@bigsamich bigsamich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good direction on test isolation and sanitizer coverage. A few things to fix before merge:

Should fix

  1. redis-start.sh:22 startup race. With --daemonize yes, the server returns before it is listening, so the immediate redis-cli ping can be refused. set -e then aborts the script, and the leftover /tmp/redis.sock makes every rerun refuse to start. Retry the ping until a deadline, as scripts/run-test-redis.py does.
  2. sanitizers.yml:33 UBSan doesn't fail the job. UBSan logs undefined behaviour and keeps running by default, so the job still passes. Add -fno-sanitize-recover=undefined.
  3. test.yml:49 flaky tests under --parallel 8. The timing-sensitive cases run alongside the CPU-heavy ones (MultiWorker, MultiReader, ConcurrentConnect). Watchdog has a 100 ms margin, and the reader tests use 5 ms and 20×5 ms windows. Mark them RUN_SERIAL or widen the timeouts.
  4. test.cpp:16 leaks keys. Each run writes under a new TEST-<pid>-<nonce>-<name> namespace and never deletes it, so long-lived servers accumulate keys. The other suites clean up, and docs/building.md says all tests do.

Smaller
5. redis-start.sh:14: the port probe binds without SO_REUSEADDR, so it reports the port as busy for about 60 s after Redis stops (TIME_WAIT). A stale socket file also blocks every start.
6. redis-start.sh:5: the script now requires python3, but the docs say Python is optional.
7. README.md:95: the new testing note sits after the License section and calls redis-start.sh legacy, while the quick start above it still uses it.
8. test.cpp:13: testBase() is the fifth copy of the unique-namespace helper. Move it into a shared test header.
9. test.cpp:240: testBase() is called about 30 times, including inside reader callbacks, which adds latency inside the tight timing windows. A TEST_F fixture that sets the base once in SetUp and deletes its keys in TearDown would also fix 4 and 8.
10. test.cpp:594: ConcurrentConnect still uses a fixed global key outside any test namespace.

@derekste

derekste commented Oct 6, 2026

Copy link
Copy Markdown
Member Author

Proposed focused CI correction at 5162997: set UBSAN_OPTIONS=halt_on_error=1 for the sanitizer suite. A real undefined-behavior control currently exits successfully under CTest; the same control fails CTest with this setting, while the healthy control passes.

This patch addresses validation integrity only. Startup-helper, test-timing/isolation and cleanup comments remain separate findings, not claimed fixed here. Please identify any remaining standalone finding that must be resolved before the agreed release landing.

@derekste
derekste requested a review from wsulli October 7, 2026 15:38
Base automatically changed from dev/stream-recovery-status to main October 7, 2026 16:04
@derekste
derekste merged commit 7f380fd into main Oct 7, 2026
3 checks passed
@derekste
derekste deleted the dev/test-tooling-hygiene branch October 7, 2026 16:07
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.

3 participants