Address Copilot review on test-control-channel-skew.sh#79
Merged
Conversation
Five small fixes from PR #78 review: * set -euo pipefail + grep-with-no-matches would short-circuit the pipeline (grep exits 1 on empty result) and bypass the explicit empty-check below. Wrap the extraction in '|| true' so the explicit failure branch can fire with the intended diagnostic. * Filtering the startup line by exact-string elapsed_secs == "0.001" is brittle — the value is computed from elapsed_ms / 1000.0 and renders as 0.0, 0.001, 0.002, etc. depending on host timing. Use a numeric threshold (>= 0.5) so the filter survives across environments. Real intervals start at 1.0+, so the threshold has comfortable headroom. * tc qdisc add fails with 'File exists' if a root qdisc is already present on lo (rerunning after an interrupted run, or when a previous test left state behind). Issue 'tc qdisc del dev lo root' unconditionally before the add, with stderr suppression and || true so the no-existing-qdisc case isn't fatal. * Stale comment said '5-second UDP test' but DURATION=8. Update to match actual behavior. * --json-stream's 'timestamp' field follows --timestamp-format which can be overridden via XFR_TIMESTAMP_FORMAT env or config-file default, producing ISO 8601 / Unix epoch strings the awk parser doesn't handle. Pass --timestamp-format relative explicitly so the test is hermetic regardless of host config. Re-validated in Docker: OLD binary (pre-NODELAY) consistently fails with rc=1 (max bunch=3); NEW binary passes with rc=0 (max bunch=2). No flake observed across runs.
There was a problem hiding this comment.
Pull request overview
Follow-up hardening for the repo-root CI regression script test-control-channel-skew.sh, addressing prior review findings to make the test more robust across reruns and environment/config differences.
Changes:
- Make
tc netemsetup idempotent by deleting any pre-existing root qdisc onlobefore adding the test shaper. - Pin
--json-streamtimestamps to a numeric relative format to keep the awk parser stable regardless of env/config defaults. - Make interval extraction resilient under
set -euo pipefailand avoid brittle startup-line filtering by switching to a numeric elapsed threshold.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Five fixes from Copilot's review on PR #78 (which already merged — these are follow-ups). All valid findings.
set -e+ grep with no matches short-circuits before the explicit empty-checkelapsed_secs == "0.001"is brittle (host-timing-dependent string)>= 0.5tc qdisc addfails with "File exists" on rerun if a prior qdisc is left overtc qdisc delfirst, with|| trueDURATION=8--json-streamtimestamp format honors env/config; could break the awk parser--timestamp-format relativeexplicitlyRe-validated in Docker: OLD binary (pre-NODELAY) → rc=1 with max bunch=3; NEW binary → rc=0 with max bunch=2. Assertion still differentiates correctly.
Test plan