Skip to content

Commit 317e71d

Browse files
etrclaude
etr
and
claude
committed
TASK-077: lock in // reason: comments for test/integ/ platform skips
Three whole-suite or near-whole-suite `#ifndef _WINDOWS` / `#ifndef DARWIN` blocks in test/integ/ were silently removing the library's Windows / Darwin portability claim from CI without any commentary on why. This change: - Adds `scripts/check-skip-rationales.sh` (lint) + `test_check_skip_rationales.sh` (10-case fixture) that enforce a `// reason:` comment within 5 lines of every `#ifndef _WINDOWS`, `#ifndef DARWIN`, or `#if !defined(_WINDOWS)` block under `test/integ/`. `#ifdef _WINDOWS` (additive coverage) is deliberately not gated. - Annotates every existing skip site (`threaded.cpp` x4, `ws_start_stop.cpp` wide skip and `custom_socket`, `authentication.cpp` digest-auth block, and `connection_state_body_residue_test.cpp`) with the required comment. - Adds `test/PORTABILITY.md` recording symptom / root cause / restoration plan per skip site; the `// reason:` lines point at the relevant section. - Adds a `ws_start_stop_suite::windows_smoke` test under `#ifdef _WINDOWS` to restore a single non-TLS HTTP GET round-trip on the MinGW64 / MSYS lanes — recovering the most valuable bit of Windows coverage without inheriting the TLS / IPv6 / SNI / PSK flake tracked in PORTABILITY.md. - Wires the lint into `verify-build.yml`'s existing `lint` lane and adds a Windows-lane assertion that grep's the test log for `windows_smoke` so a future build-flag tweak that suppresses the `#ifdef _WINDOWS` branch fails CI explicitly rather than silently erasing coverage. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 858dfde commit 317e71d

8 files changed

Lines changed: 521 additions & 4 deletions

File tree

‎.github/workflows/verify-build.yml‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -770,6 +770,16 @@ jobs:
770770
run: ./scripts/check-warning-suppressions.sh
771771
if: ${{ matrix.build-type == 'lint' && matrix.os-type == 'ubuntu' }}
772772

773+
- name: Run skip-rationale gate
774+
# TASK-077: forbid `#ifndef _WINDOWS`, `#ifndef DARWIN`, or
775+
# `#if !defined(_WINDOWS)` blocks in `test/integ/` that lack a
776+
# `// reason:` comment pointing at `test/PORTABILITY.md` (or a
777+
# follow-up task). Without this gate, whole-suite skips silently
778+
# drift on the Windows / Darwin CI lanes and the library's
779+
# portability claim quietly decays.
780+
run: ./scripts/check-skip-rationales.sh
781+
if: ${{ matrix.build-type == 'lint' && matrix.os-type == 'ubuntu' }}
782+
773783
- name: Run libhttpserver configure
774784
# TASK-038: fixed CXXLAGS -> CXXFLAGS typo so C++ TUs are instrumented by the sanitizer matrix.
775785
run: |
@@ -1014,6 +1024,26 @@ jobs:
10141024
cat test/test-suite.log ;
10151025
if: ${{ failure() && matrix.build-type != 'iwyu' && matrix.compiler-family != 'arm-cross' && matrix.build-type != 'header-hygiene' }}
10161026

1027+
- name: Verify Windows smoke test ran (TASK-077)
1028+
# TASK-077: assert the new `ws_start_stop_suite::windows_smoke` test
1029+
# is actually being compiled and executed on the MinGW64 / MSYS lanes
1030+
# so that the basic daemon-start-accept-one-GET path is observed end
1031+
# to end on Windows. Without this, a future build-flag tweak that
1032+
# inadvertently turns off the `#ifdef _WINDOWS` branch would silently
1033+
# erase the Windows round-trip coverage.
1034+
shell: bash
1035+
run: |
1036+
cd build
1037+
if [ ! -f test/ws_start_stop.log ]; then
1038+
echo "ERROR: test/ws_start_stop.log is missing — windows_smoke could not be verified" >&2
1039+
exit 1
1040+
fi
1041+
if ! grep -E "windows_smoke" test/ws_start_stop.log; then
1042+
echo "ERROR: windows_smoke test did not appear in test/ws_start_stop.log" >&2
1043+
exit 1
1044+
fi
1045+
if: ${{ matrix.os-type == 'windows' }}
1046+
10171047
- name: Run Valgrind checks
10181048
run: |
10191049
cd build ;

‎scripts/check-skip-rationales.sh‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
#!/usr/bin/env bash
2+
#
3+
# check-skip-rationales.sh — guard against unexplained platform skips in
4+
# the integration test suite (TASK-077).
5+
#
6+
# Three whole-suite or near-whole-suite skips in `test/integ/` were
7+
# silently removing the library's Windows/Darwin portability claim from
8+
# CI. The audit captured in `test/PORTABILITY.md` records every known
9+
# skip site, its symptom, root cause, and restoration plan. This gate
10+
# enforces that any future skip carries a `// reason:` comment that
11+
# points at PORTABILITY.md (or a follow-up task) so the next reader
12+
# isn't left guessing why a whole suite vanishes on Windows.
13+
#
14+
# Watched files: every `.cpp` / `.hpp` under `test/integ/`, discovered
15+
# at runtime via `find` — the broad scan keeps new test files under the
16+
# same gate without requiring a manual list update (same pattern as
17+
# `check-warning-suppressions.sh`).
18+
#
19+
# Detection logic:
20+
# 1. Lines matching either
21+
# ^#ifndef[[:space:]]+(_WINDOWS|DARWIN)
22+
# or
23+
# ^#if[[:space:]]+!defined\([[:space:]]*(_WINDOWS|DARWIN)[[:space:]]*\)
24+
# are candidate "skip" directives. Note the deliberate asymmetry:
25+
# `#ifdef _WINDOWS` / `#ifdef DARWIN` blocks (additive coverage,
26+
# e.g. a Windows-only smoke variant) do NOT require a `// reason:`
27+
# comment — those are restoring coverage, not removing it.
28+
# 2. A candidate is compliant iff any of the 5 lines immediately
29+
# preceding the directive contains the substring `// reason:`
30+
# (case-sensitive).
31+
# 3. Any candidate that fails the bracketing check is reported and
32+
# the script exits 1.
33+
#
34+
# Exit codes:
35+
# 0 no violations
36+
# 1 one or more skip directives lack a `// reason:` comment
37+
set -euo pipefail
38+
39+
REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)"
40+
cd "$REPO_ROOT"
41+
42+
WATCHED_FILES=()
43+
while IFS= read -r f; do
44+
WATCHED_FILES+=("$f")
45+
done < <(find test/integ -type f \( -name '*.cpp' -o -name '*.hpp' \) 2>/dev/null | sort)
46+
47+
echo "check-skip-rationales: scanning ${#WATCHED_FILES[@]} file(s) under test/integ/"
48+
49+
if [ "${#WATCHED_FILES[@]}" -eq 0 ]; then
50+
echo "check-skip-rationales: PASS — no test/integ/ files found"
51+
exit 0
52+
fi
53+
54+
# Matches:
55+
# #ifndef _WINDOWS
56+
# #ifndef DARWIN
57+
# #if !defined(_WINDOWS) (with or without surrounding spaces)
58+
# #if !defined(_WINDOWS) && ...
59+
# #if !defined(DARWIN) && ...
60+
# Does NOT match:
61+
# #ifdef _WINDOWS (additive coverage)
62+
# #define _WINDOWS (synthetic macro for testing)
63+
SKIP_REGEX='^#ifndef[[:space:]]+(_WINDOWS|DARWIN)([[:space:]]|$)|^#if[[:space:]]+!defined\([[:space:]]*(_WINDOWS|DARWIN)[[:space:]]*\)'
64+
65+
violations=0
66+
for file in "${WATCHED_FILES[@]}"; do
67+
if [ ! -f "$file" ]; then
68+
echo " $file: missing — watched file no longer present" >&2
69+
violations=$((violations + 1))
70+
continue
71+
fi
72+
73+
while IFS=: read -r lineno directive; do
74+
# Look at the 5 lines immediately preceding the directive for a
75+
# `// reason:` substring. `awk` keeps this single-pass and
76+
# avoids spawning `sed`/`head`/`tail` per directive.
77+
has_reason=$(awk -v target="$lineno" '
78+
NR >= target - 5 && NR < target {
79+
if (index($0, "// reason:") > 0) { found = 1 }
80+
}
81+
NR >= target { exit }
82+
END { print (found ? 1 : 0) }
83+
' "$file")
84+
85+
if [ "$has_reason" != "1" ]; then
86+
# Trim leading whitespace from the directive for the report.
87+
trimmed_directive="${directive#"${directive%%[![:space:]]*}"}"
88+
echo " $file:$lineno: missing // reason: comment for '$trimmed_directive'" >&2
89+
violations=$((violations + 1))
90+
fi
91+
done < <(grep -nE "$SKIP_REGEX" "$file" || true)
92+
done
93+
94+
if [ "$violations" -gt 0 ]; then
95+
echo "check-skip-rationales: FAIL — $violations skip directive(s) without // reason: comment" >&2
96+
echo " add a '// reason: ...' line within the 5 lines preceding the directive that names the platform limitation" >&2
97+
echo " and points at test/PORTABILITY.md (or a follow-up task)" >&2
98+
exit 1
99+
fi
100+
101+
echo "check-skip-rationales: PASS — every skip directive carries a // reason: comment"

‎scripts/test_check_skip_rationales.sh‎

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
#!/usr/bin/env bash
2+
# Unit test for check-skip-rationales.sh
3+
#
4+
# TASK-077: every `#ifndef _WINDOWS` / `#ifndef DARWIN` (and the equivalent
5+
# `#if !defined(_WINDOWS)` form) block in `test/integ/` must carry a
6+
# `// reason:` comment within the 5 lines immediately preceding the directive
7+
# so that the underlying platform limitation is recorded in
8+
# `test/PORTABILITY.md` or a follow-up task.
9+
#
10+
# Tests use a temporary directory tree that mimics test/integ/ so the script
11+
# can be exercised against synthetic fixtures without touching the live tree.
12+
set -euo pipefail
13+
14+
PASS=0
15+
FAIL=0
16+
17+
ok() { echo " OK: $1"; PASS=$((PASS + 1)); }
18+
fail() { echo " FAIL: $1"; FAIL=$((FAIL + 1)); }
19+
20+
SCRIPT="$(cd "$(dirname "$0")" && pwd)/check-skip-rationales.sh"
21+
22+
TMPDIR_BASE="$(mktemp -d)"
23+
trap 'rm -rf "$TMPDIR_BASE"' EXIT
24+
25+
FAKE_REPO="$TMPDIR_BASE/repo"
26+
mkdir -p "$FAKE_REPO/scripts" "$FAKE_REPO/test/integ"
27+
28+
cp "$SCRIPT" "$FAKE_REPO/scripts/check-skip-rationales.sh"
29+
chmod +x "$FAKE_REPO/scripts/check-skip-rationales.sh"
30+
31+
reset_repo() {
32+
rm -f "$FAKE_REPO"/test/integ/*.cpp "$FAKE_REPO"/test/integ/*.hpp
33+
}
34+
35+
# Test 1: No `#ifndef _WINDOWS` directives — should exit 0.
36+
reset_repo
37+
cat > "$FAKE_REPO/test/integ/clean.cpp" <<'EOF'
38+
// No platform skips at all.
39+
int main() { return 0; }
40+
EOF
41+
if (cd "$FAKE_REPO" && bash scripts/check-skip-rationales.sh 2>/dev/null); then
42+
ok "test 1: no platform skips exits 0"
43+
else
44+
fail "test 1: no platform skips should exit 0"
45+
fi
46+
47+
# Test 2: `#ifndef _WINDOWS` with `// reason:` on the immediately preceding
48+
# line — should exit 0.
49+
reset_repo
50+
cat > "$FAKE_REPO/test/integ/adjacent.cpp" <<'EOF'
51+
// Some code
52+
// reason: MHD's Windows path can't drive INTERNAL_SELECT thread pools.
53+
#ifndef _WINDOWS
54+
int x = 1;
55+
#endif
56+
EOF
57+
if (cd "$FAKE_REPO" && bash scripts/check-skip-rationales.sh 2>/dev/null); then
58+
ok "test 2: adjacent reason exits 0"
59+
else
60+
fail "test 2: adjacent reason should exit 0"
61+
fi
62+
63+
# Test 3: `#ifndef _WINDOWS` with `// reason:` 3 lines above (blank lines
64+
# allowed between) — should exit 0.
65+
reset_repo
66+
cat > "$FAKE_REPO/test/integ/within_window.cpp" <<'EOF'
67+
// reason: see test/PORTABILITY.md §threaded.cpp.
68+
69+
// (blank line above is fine)
70+
#ifndef _WINDOWS
71+
int x = 1;
72+
#endif
73+
EOF
74+
if (cd "$FAKE_REPO" && bash scripts/check-skip-rationales.sh 2>/dev/null); then
75+
ok "test 3: reason 3 lines above exits 0"
76+
else
77+
fail "test 3: reason 3 lines above should exit 0"
78+
fi
79+
80+
# Test 4: `#ifndef _WINDOWS` with `// reason:` 6 lines above (outside window)
81+
# — should exit 1.
82+
reset_repo
83+
cat > "$FAKE_REPO/test/integ/outside_window.cpp" <<'EOF'
84+
// reason: this comment sits more than 5 lines above the directive.
85+
//
86+
//
87+
//
88+
//
89+
//
90+
#ifndef _WINDOWS
91+
int x = 1;
92+
#endif
93+
EOF
94+
if ! (cd "$FAKE_REPO" && bash scripts/check-skip-rationales.sh 2>/dev/null); then
95+
ok "test 4: reason outside window exits 1"
96+
else
97+
fail "test 4: reason outside window should exit 1"
98+
fi
99+
100+
# Test 5: `#ifndef _WINDOWS` with no `// reason:` anywhere — should exit 1.
101+
reset_repo
102+
cat > "$FAKE_REPO/test/integ/bare.cpp" <<'EOF'
103+
#ifndef _WINDOWS
104+
int x = 1;
105+
#endif
106+
EOF
107+
if ! (cd "$FAKE_REPO" && bash scripts/check-skip-rationales.sh 2>/dev/null); then
108+
ok "test 5: bare _WINDOWS exits 1"
109+
else
110+
fail "test 5: bare _WINDOWS should exit 1"
111+
fi
112+
113+
# Test 6: `#ifndef DARWIN` with no `// reason:` — should exit 1
114+
# (verifies DARWIN is also covered).
115+
reset_repo
116+
cat > "$FAKE_REPO/test/integ/darwin_bare.cpp" <<'EOF'
117+
#ifndef DARWIN
118+
int x = 1;
119+
#endif
120+
EOF
121+
if ! (cd "$FAKE_REPO" && bash scripts/check-skip-rationales.sh 2>/dev/null); then
122+
ok "test 6: bare DARWIN exits 1"
123+
else
124+
fail "test 6: bare DARWIN should exit 1"
125+
fi
126+
127+
# Test 7: `#ifdef _WINDOWS` (opposite polarity — additive, not a skip)
128+
# does NOT require `// reason:`; should exit 0.
129+
reset_repo
130+
cat > "$FAKE_REPO/test/integ/additive.cpp" <<'EOF'
131+
// No reason comment here.
132+
#ifdef _WINDOWS
133+
int x = 1;
134+
#endif
135+
EOF
136+
if (cd "$FAKE_REPO" && bash scripts/check-skip-rationales.sh 2>/dev/null); then
137+
ok "test 7: #ifdef _WINDOWS (additive) exits 0"
138+
else
139+
fail "test 7: #ifdef _WINDOWS (additive) should exit 0"
140+
fi
141+
142+
# Test 8: `#if !defined(_WINDOWS) && defined(HAVE_DAUTH)` form (the
143+
# authentication.cpp digest-auth guard) without a reason — should exit 1.
144+
reset_repo
145+
cat > "$FAKE_REPO/test/integ/if_not_defined.cpp" <<'EOF'
146+
#if !defined(_WINDOWS) && defined(HAVE_DAUTH)
147+
int x = 1;
148+
#endif
149+
EOF
150+
if ! (cd "$FAKE_REPO" && bash scripts/check-skip-rationales.sh 2>/dev/null); then
151+
ok "test 8: #if !defined(_WINDOWS) bare exits 1"
152+
else
153+
fail "test 8: #if !defined(_WINDOWS) bare should exit 1"
154+
fi
155+
156+
# Test 9: same `#if !defined(_WINDOWS)` form with `// reason:` adjacent — should exit 0.
157+
reset_repo
158+
cat > "$FAKE_REPO/test/integ/if_not_defined_ok.cpp" <<'EOF'
159+
// reason: MinGW64 curl's --digest parser rejects the MHD challenge.
160+
#if !defined(_WINDOWS) && defined(HAVE_DAUTH)
161+
int x = 1;
162+
#endif
163+
EOF
164+
if (cd "$FAKE_REPO" && bash scripts/check-skip-rationales.sh 2>/dev/null); then
165+
ok "test 9: #if !defined(_WINDOWS) with reason exits 0"
166+
else
167+
fail "test 9: #if !defined(_WINDOWS) with reason should exit 0"
168+
fi
169+
170+
# Test 10: the live repo's test/integ/ — should exit 0 after rationale
171+
# comments have been added to every skip site. Skipped if the live tree
172+
# does not exist or is in the middle of the implementation (the
173+
# regression guard runs against the real repo, not this fake one).
174+
REAL_REPO="$(cd "$(dirname "$0")/.." && pwd)"
175+
if [ -d "$REAL_REPO/test/integ" ]; then
176+
if (cd "$REAL_REPO" && bash scripts/check-skip-rationales.sh 2>/dev/null); then
177+
ok "test 10: live test/integ/ tree exits 0"
178+
else
179+
fail "test 10: live test/integ/ tree should exit 0 (add // reason: comments to all #ifndef _WINDOWS/DARWIN blocks)"
180+
fi
181+
fi
182+
183+
echo ""
184+
echo "Results: $PASS passed, $FAIL failed"
185+
[ "$FAIL" -eq 0 ]

0 commit comments

Comments
 (0)