Rework build workflow and enhance unit test coverage#3
Merged
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRestructures CI into separate debug and release workflows (and deprecates the old build.yml), adds a local-build orchestration script, expands and wires extensive unit/integration tests and test helpers, and applies targeted buffer/timeval value-initializations across network code to avoid uninitialized reads. ChangesCI / Local build / Test wiring
Test helpers and test suites
Network safety and small API-adjacent edits
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
tests/test_utils.cpp (1)
119-127: ⚡ Quick winUse RAII for
g_no_colorstate restoration in tests.If a
REQUIREfails before manual restore, global color state can leak into later tests.Proposed guard pattern
namespace { +struct NoColorGuard { + bool prev = g_no_color; + ~NoColorGuard() { g_no_color = prev; } +};TEST_CASE("col returns empty string when no_color is true") { - const bool saved = g_no_color; + NoColorGuard guard; g_no_color = true; REQUIRE(std::string(col(C::RED)).empty()); REQUIRE(std::string(col(C::BOLD)).empty()); g_no_color = false; REQUIRE_FALSE(std::string(col(C::RED)).empty()); - g_no_color = saved; }Also applies to: 318-322, 333-339
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_utils.cpp` around lines 119 - 127, Replace the manual save/restore of the global g_no_color in the TEST_CASEs with a small RAII guard that saves the current g_no_color in its constructor and restores it in its destructor; e.g., add a helper class (e.g., NoColorGuard) that captures g_no_color on construction and restores it on destruction, then instantiate it at the start of the tests that manipulate g_no_color (the TEST_CASE that calls col(C::RED) and col(C::BOLD) and the other occurrences mentioned) so the global state is always restored even if a REQUIRE fails.tests/test_openssl_runtime.cpp (1)
24-28: ⚡ Quick winConcurrency is not actually tested in this case.
Line 24-28 performs sequential calls, so it won’t catch thread-safety regressions despite the test name.
Proposed test update
TEST_CASE("openssl_runtime_init is safe to call multiple times concurrently") { - REQUIRE(openssl_runtime_init()); - REQUIRE(openssl_runtime_init()); - REQUIRE(openssl_runtime_init()); + bool ok1 = false, ok2 = false, ok3 = false; + std::thread t1([&] { ok1 = openssl_runtime_init(); }); + std::thread t2([&] { ok2 = openssl_runtime_init(); }); + std::thread t3([&] { ok3 = openssl_runtime_init(); }); + t1.join(); + t2.join(); + t3.join(); + REQUIRE(ok1); + REQUIRE(ok2); + REQUIRE(ok3); }// add near the includes `#include` <thread>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_openssl_runtime.cpp` around lines 24 - 28, The test named TEST_CASE should actually exercise concurrency: include <thread> and modify the test body to spawn multiple std::thread workers that concurrently call openssl_runtime_init(), join them, and assert each call returned true (or aggregate results and REQUIRE all succeeded); keep the TEST_CASE name and ensure you create enough threads (e.g., std::thread vector) to reliably surface thread-safety issues for openssl_runtime_init().tests/test_tcp_async_scan.cpp (1)
114-130: 💤 Low valueTest validates sorting with single port only.
The test is named "results are sorted by port" but constructs a port list with only one port (line 123), so the sorting loop at lines 127-129 won't meaningfully validate sort order. Sorting is properly validated elsewhere (e.g., line 220-222 with 40 ports), but this test name is misleading.
Consider renaming or adding ports
Either rename the test to reflect single-port behavior:
-TEST_CASE("scan_tcp_async results are sorted by port") { +TEST_CASE("scan_tcp_async single port result structure") {Or add multiple open ports to actually test sorting:
- const std::vector<int> ports = {server.port()}; + testnet::TcpMultiShotServer server2(/*...*/); + const std::vector<int> ports = {server2.port(), server.port()};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_tcp_async_scan.cpp` around lines 114 - 130, The test TEST_CASE("scan_tcp_async results are sorted by port") only provides a single port in the ports vector (constructed from testnet::TcpOneShotServer::port()), so the sort assertion is vacuous; either rename the test to indicate single-port behavior or modify it to provide multiple ports (e.g., spin up multiple testnet::TcpOneShotServer instances or gather multiple ports, build a vector<int> with >1 ports, call scan_tcp_async("127.0.0.1", ports, 32, 300, &stats) and then keep the existing loop that asserts result[i-1].port <= result[i].port) to actually validate sorting.tests/test_vpn_probes.cpp (1)
19-126: 💤 Low valueConsider extracting protocol constants.
The WireGuard packet size (148 bytes) and AmneziaWG junk prefix cap (64 bytes) appear as magic numbers throughout the tests. Named constants would improve readability and maintainability.
Example refactor with named constants
+namespace { +constexpr std::size_t kWireGuardPacketSize = 148; +constexpr std::size_t kAmneziaWGMaxJunkPrefix = 64; + struct UdpJitterGuard { bool jitter = g_udp_jitter; ~UdpJitterGuard() { g_udp_jitter = jitter; } }; +} // namespaceThen use throughout:
- REQUIRE(observed_size.load(std::memory_order_relaxed) == 148); + REQUIRE(observed_size.load(std::memory_order_relaxed) == kWireGuardPacketSize);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_vpn_probes.cpp` around lines 19 - 126, Extract the magic numbers into named constants (e.g., constexpr std::size_t WIREGUARD_PACKET_SIZE = 148; constexpr std::size_t AMNEZIAWG_JUNK_CAP = 64;) and replace literal 148 and 64 usages in the tests that exercise wireguard_probe and amneziawg_probe (the TEST_CASEs asserting observed_size and observed_marker/observed_first) with these constants; ensure the constants are visible to the test file (add them at the top of the test file or in a shared test_header) and update assertions that compute expected lengths (like 64 + 148 and mid-range prefix checks) to use the named constants so the intent is clear and maintainable..github/workflows/release.yml (1)
15-21: 💤 Low valueConsider
fail-fast: falsefor the release matrix.The default
fail-fast: truewill cancel the in-flight Windows job the moment Linux fails (or vice-versa). Since the downstreamreleasejob needs both artifacts, you'd typically prefer to let both platforms run to completion so you can see all failures at once and avoid a second tag push if only one side broke.♻️ Proposed change
strategy: + fail-fast: false matrix: include: - os: ubuntu-latest generator: Ninja - os: windows-latest generator: "Visual Studio 17 2022"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml around lines 15 - 21, The release matrix currently uses the default fail-fast behavior which will cancel in-flight platform jobs; update the GitHub Actions job strategy by adding fail-fast: false under the existing strategy: matrix block (next to the matrix/include entries) so both generators (the entries under include) run to completion; modify the job that defines strategy/matrix to include fail-fast: false to ensure both ubuntu-latest and windows-latest jobs finish even if one fails.local_build.sh (2)
86-88: 💤 Low valueConsider narrowing container privileges for the sanitizer runs.
--privileged --network=hostgrants the container full host capabilities, mounts, and network namespace. For ASan/TSan/MSan you typically only need--cap-add=SYS_PTRACEand disabling ASLR (--security-opt seccomp=unconfined).--network=hostis unnecessary unless the test suite binds to host ports —network_test_helpers.hreserves ephemeral ports on loopback, which works in a default bridge network. Since this is a local developer script, the blast radius is contained, but tightening this avoids surprising side effects on the host.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@local_build.sh` around lines 86 - 88, The podman invocation in the for loop in local_build.sh uses overly broad flags (--privileged --network=host); update the podman run command inside the for SAN in asan-ubsan tsan msan loop to remove --privileged and --network=host, add only needed capabilities such as --cap-add=SYS_PTRACE and --security-opt seccomp=unconfined to enable sanitizers, and only reintroduce --network=host when a specific test requires host networking; adjust the podman run invocation that accepts "$SAN" "$OPENSSL_VERSION" "$STATIC_FLAG" accordingly.
6-13: 💤 Low value
shiftinsidefor arg in "$@"is a no-op for the loop iteration.Bash snapshots the positional parameters at loop entry, so
shifthere neither advances iteration nor cleans up$@for the rest of the script (you re-process the same flag every pass). It's harmless today, but it implies intent that the construct doesn't deliver — awhile/caseloop would be clearer if you ever add more flags. Optional cleanup.♻️ Optional rewrite
-for arg in "$@"; do - case $arg in - --keep) - KEEP_ARTIFACTS=1 - shift - ;; - esac -done +while [ $# -gt 0 ]; do + case "$1" in + --keep) + KEEP_ARTIFACTS=1 + shift + ;; + *) + echo "Unknown argument: $1" >&2 + exit 2 + ;; + esac +done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@local_build.sh` around lines 6 - 13, The for-loop over "$@" that calls shift is misleading because shift does not affect the loop's snapshot of positional parameters; replace the "for arg in \"$@\"; do ... case ... shift ... done" construct with a positional-parameter consuming loop that tests "$#" and switches on "$1" so shift actually advances parsing; update the case branch that sets KEEP_ARTIFACTS to set the variable when encountering "--keep" and then call shift to consume that option, thereby ensuring flags are not reprocessed and future flags can be handled correctly..github/workflows/debug.yml (1)
180-180: 💤 Low valuePin
actions/cacheto a commit SHA for consistency.All other third-party actions in this workflow are pinned by commit SHA (e.g.
actions/checkout@11bd71...,actions/upload-artifact@4cec3d8...), butactions/cache@v4is pinned only to a major-version tag, which is mutable and weakens the supply-chain posture you've established. Pin it to a commit SHA to match.Also applies to: 250-250, 319-319
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/debug.yml at line 180, Replace the mutable tag usage "uses: actions/cache@v4" with a pinned commit SHA (e.g. "uses: actions/cache@<commit-sha>") to match the other actions; update every occurrence of the cache action (the entries currently using actions/cache@v4, including the occurrences corresponding to the other mentions) so they all reference a specific commit SHA instead of the version tag.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/debug.yml:
- Around line 41-43: The find invocation used to feed files to clang-tidy is
unquoted and can suffer word-splitting on paths with whitespace; update the Run
clang-tidy step so the output of find is passed safely (use find ... -exec ...
{} + or use find ... -print0 piped to xargs -0) when invoking clang-tidy,
replacing the current $(find src -name '*.cpp' -print) usage to ensure paths
with spaces are handled correctly.
In `@local_build.sh`:
- Around line 46-50: The current CPP_FILES expansion is word-splitting fragile;
replace the gather-and-expand approach using the CPP_FILES variable with a
NUL-delimited pipeline: use find ... -print0 and pipe to xargs -0 to invoke
clang-tidy for each matched file (i.e., change the logic around the CPP_FILES
variable and the clang-tidy call so it reads from find -print0 | xargs -0
clang-tidy -p build --checks='clang-analyzer-*,clang-diagnostic-*'
-warnings-as-errors='*'). Ensure you remove the unquoted $CPP_FILES usage and
use the NUL-safe find/xargs pipeline instead so paths with whitespace are
handled correctly.
In `@tests/test_openssl_runtime.cpp`:
- Around line 104-107: The test currently only inspects `err` when
`ssl_attach_socket(ssl, s, &err)` returns false, so the failure path might not
be exercised; change the assertions to first assert the call actually failed
(e.g., require `ok` is false using `REQUIRE_FALSE(ok)` or equivalent) and then
assert `err` is non-empty; target the `ssl_attach_socket` call and the
`ok`/`err` variables in the test to enforce both the failure result and that an
error message was produced.
---
Nitpick comments:
In @.github/workflows/debug.yml:
- Line 180: Replace the mutable tag usage "uses: actions/cache@v4" with a pinned
commit SHA (e.g. "uses: actions/cache@<commit-sha>") to match the other actions;
update every occurrence of the cache action (the entries currently using
actions/cache@v4, including the occurrences corresponding to the other mentions)
so they all reference a specific commit SHA instead of the version tag.
In @.github/workflows/release.yml:
- Around line 15-21: The release matrix currently uses the default fail-fast
behavior which will cancel in-flight platform jobs; update the GitHub Actions
job strategy by adding fail-fast: false under the existing strategy: matrix
block (next to the matrix/include entries) so both generators (the entries under
include) run to completion; modify the job that defines strategy/matrix to
include fail-fast: false to ensure both ubuntu-latest and windows-latest jobs
finish even if one fails.
In `@local_build.sh`:
- Around line 86-88: The podman invocation in the for loop in local_build.sh
uses overly broad flags (--privileged --network=host); update the podman run
command inside the for SAN in asan-ubsan tsan msan loop to remove --privileged
and --network=host, add only needed capabilities such as --cap-add=SYS_PTRACE
and --security-opt seccomp=unconfined to enable sanitizers, and only reintroduce
--network=host when a specific test requires host networking; adjust the podman
run invocation that accepts "$SAN" "$OPENSSL_VERSION" "$STATIC_FLAG"
accordingly.
- Around line 6-13: The for-loop over "$@" that calls shift is misleading
because shift does not affect the loop's snapshot of positional parameters;
replace the "for arg in \"$@\"; do ... case ... shift ... done" construct with a
positional-parameter consuming loop that tests "$#" and switches on "$1" so
shift actually advances parsing; update the case branch that sets KEEP_ARTIFACTS
to set the variable when encountering "--keep" and then call shift to consume
that option, thereby ensuring flags are not reprocessed and future flags can be
handled correctly.
In `@tests/test_openssl_runtime.cpp`:
- Around line 24-28: The test named TEST_CASE should actually exercise
concurrency: include <thread> and modify the test body to spawn multiple
std::thread workers that concurrently call openssl_runtime_init(), join them,
and assert each call returned true (or aggregate results and REQUIRE all
succeeded); keep the TEST_CASE name and ensure you create enough threads (e.g.,
std::thread vector) to reliably surface thread-safety issues for
openssl_runtime_init().
In `@tests/test_tcp_async_scan.cpp`:
- Around line 114-130: The test TEST_CASE("scan_tcp_async results are sorted by
port") only provides a single port in the ports vector (constructed from
testnet::TcpOneShotServer::port()), so the sort assertion is vacuous; either
rename the test to indicate single-port behavior or modify it to provide
multiple ports (e.g., spin up multiple testnet::TcpOneShotServer instances or
gather multiple ports, build a vector<int> with >1 ports, call
scan_tcp_async("127.0.0.1", ports, 32, 300, &stats) and then keep the existing
loop that asserts result[i-1].port <= result[i].port) to actually validate
sorting.
In `@tests/test_utils.cpp`:
- Around line 119-127: Replace the manual save/restore of the global g_no_color
in the TEST_CASEs with a small RAII guard that saves the current g_no_color in
its constructor and restores it in its destructor; e.g., add a helper class
(e.g., NoColorGuard) that captures g_no_color on construction and restores it on
destruction, then instantiate it at the start of the tests that manipulate
g_no_color (the TEST_CASE that calls col(C::RED) and col(C::BOLD) and the other
occurrences mentioned) so the global state is always restored even if a REQUIRE
fails.
In `@tests/test_vpn_probes.cpp`:
- Around line 19-126: Extract the magic numbers into named constants (e.g.,
constexpr std::size_t WIREGUARD_PACKET_SIZE = 148; constexpr std::size_t
AMNEZIAWG_JUNK_CAP = 64;) and replace literal 148 and 64 usages in the tests
that exercise wireguard_probe and amneziawg_probe (the TEST_CASEs asserting
observed_size and observed_marker/observed_first) with these constants; ensure
the constants are visible to the test file (add them at the top of the test file
or in a shared test_header) and update assertions that compute expected lengths
(like 64 + 148 and mid-range prefix checks) to use the named constants so the
intent is clear and maintainable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f22a4e78-8215-46b6-b77e-61027b36e1f8
📒 Files selected for processing (28)
.github/workflows/build.yml.github/workflows/debug.yml.github/workflows/release.yml.gitignoreCMakeLists.txtlocal_build.shsrc/network/http_client.cppsrc/network/https_probe.cppsrc/network/j3_probes.cppsrc/network/service_probes.cppsrc/network/tcp_async_scan.cppsrc/network/tcp_scanner.cppsrc/network/udp_scanner.cpptests/CMakeLists.txttests/network_test_helpers.htests/test_brand_cert.cpptests/test_ct_check.cpptests/test_dns.cpptests/test_http_client.cpptests/test_j3_probes.cpptests/test_openssl_runtime.cpptests/test_port_scan.cpptests/test_service_probes.cpptests/test_tcp_async_scan.cpptests/test_tcp_scanner.cpptests/test_udp_scanner.cpptests/test_utils.cpptests/test_vpn_probes.cpp
Comment on lines
+104
to
+107
| const bool ok = ssl_attach_socket(ssl, s, &err); | ||
| if (!ok) { | ||
| REQUIRE_FALSE(err.empty()); | ||
| } |
There was a problem hiding this comment.
Closed-socket failure path is not strictly asserted.
Line 105-107 only checks err when ok == false, so this test can pass even if no failure path is exercised.
Proposed assertion tightening
std::string err;
const bool ok = ssl_attach_socket(ssl, s, &err);
- if (!ok) {
- REQUIRE_FALSE(err.empty());
- }
+ REQUIRE_FALSE(ok);
+ REQUIRE_FALSE(err.empty());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const bool ok = ssl_attach_socket(ssl, s, &err); | |
| if (!ok) { | |
| REQUIRE_FALSE(err.empty()); | |
| } | |
| const bool ok = ssl_attach_socket(ssl, s, &err); | |
| REQUIRE_FALSE(ok); | |
| REQUIRE_FALSE(err.empty()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_openssl_runtime.cpp` around lines 104 - 107, The test currently
only inspects `err` when `ssl_attach_socket(ssl, s, &err)` returns false, so the
failure path might not be exercised; change the assertions to first assert the
call actually failed (e.g., require `ok` is false using `REQUIRE_FALSE(ok)` or
equivalent) and then assert `err` is non-empty; target the `ssl_attach_socket`
call and the `ok`/`err` variables in the test to enforce both the failure result
and that an error message was produced.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@local_build.sh`:
- Around line 6-15: The script's argument parsing currently swallows unknown
flags in the case * branch which lets typos like --kep proceed and still trigger
the EXIT trap that deletes artifacts; update the while/case parsing so that the
default (*) branch treats unknown options as errors: print a clear error/usage
message including the offending flag ($1), exit non‑zero, and do not shift past
it (so the caller sees the failure). Reference the argument parsing case block
and the KEEP_ARTIFACTS variable and ensure the EXIT trap behavior remains
unchanged when a proper --keep is provided.
- Line 32: The cached OpenSSL install path is keyed only by sanitizer name
(OPENSSL_PREFIX) so bumping OPENSSL_VERSION won’t trigger a rebuild; update the
variable that defines OPENSSL_PREFIX to include the OPENSSL_VERSION (e.g.,
append "-${OPENSSL_VERSION}" or similar) and ensure any directory existence
checks that reference OPENSSL_PREFIX (the code that decides whether to
download/build OpenSSL) will now reflect version changes so the script
redownloads/rebuilds when OPENSSL_VERSION is bumped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21af0f9f-bd7f-48f8-9b67-fb91a17826bb
📒 Files selected for processing (6)
.github/workflows/debug.ymllocal_build.shtests/test_openssl_runtime.cpptests/test_tcp_async_scan.cpptests/test_utils.cpptests/test_vpn_probes.cpp
✅ Files skipped from review due to trivial changes (1)
- tests/test_vpn_probes.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_utils.cpp
- tests/test_tcp_async_scan.cpp
- tests/test_openssl_runtime.cpp
- .github/workflows/debug.yml
Owner
Author
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
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.
Summary by CodeRabbit
Chores
Bug Fixes
Tests