Skip to content

Commit 4a9a2b5

Browse files
etrclaude
etr
and
claude
committed
Merge TASK-077: lock in // reason: comments for test/integ/ platform skips
Adds `scripts/check-skip-rationales.sh` lint (with a 10-case fixture) that fails CI if a `#ifndef _WINDOWS` / `#ifndef DARWIN` / `#if !defined(_WINDOWS)` block under `test/integ/` lacks a `// reason:` comment within 5 lines. Annotates every existing skip site, adds `test/PORTABILITY.md` (symptom / root cause / restoration plan per skip), and restores a single Windows non-TLS HTTP smoke test (`ws_start_stop_suite::windows_smoke`) to recover the most valuable bit of Windows coverage. Wires the lint into the verify-build.yml lint lane and adds a Windows-lane assertion that grep's the test log for `windows_smoke` so a future build-flag tweak that silently suppresses the `#ifdef _WINDOWS` branch fails CI. Includes the status flip to Done and 27 persisted minor review findings (0 critical, 0 major) for future cleanup sweeps. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2 parents 858dfde + 563f86d commit 4a9a2b5

11 files changed

Lines changed: 643 additions & 11 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 ]

‎specs/tasks/M7-v2-cleanup/TASK-077.md‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,11 @@ Three whole-suite or near-whole-suite skips remove the library's portability cla
1414
Either land Windows/Darwin variants or document and gate the exclusion behind a CI flag that asserts intent rather than letting silent skips drift.
1515

1616
**Action Items:**
17-
- [ ] For `threaded.cpp`: write a Windows-shaped variant of the suite (matching the I/O completion port semantics MHD uses on Windows) under `#ifdef _WINDOWS`, or document why no Windows variant is feasible and capture the gap in `test/PORTABILITY.md`.
18-
- [ ] For `ws_start_stop.cpp` Windows skip: same — variant suite under `#ifdef _WINDOWS`, or PORTABILITY.md note. Comment must explain *why* MHD's Windows path can't drive this scenario.
19-
- [ ] For the postponed digest-auth Windows tests in `ws_start_stop.cpp:176-180` and `authentication.cpp:176-180`: either port them or document the upstream MHD-on-Windows limitation. If coordinated with TASK-062 (Digest RFC-7616), bring up there.
20-
- [ ] For `ws_start_stop.cpp:337` `custom_socket` Darwin skip: add a comment naming the macOS-specific limitation, or port. Comment must reference the specific Darwin syscall behaviour that fails.
21-
- [ ] Add a `scripts/check-skip-rationales.sh` lint that fails CI if a `#ifndef _WINDOWS` or `#ifndef DARWIN` block in `test/integ/` lacks a `// reason:` comment.
17+
- [x] For `threaded.cpp`: write a Windows-shaped variant of the suite (matching the I/O completion port semantics MHD uses on Windows) under `#ifdef _WINDOWS`, or document why no Windows variant is feasible and capture the gap in `test/PORTABILITY.md`.
18+
- [x] For `ws_start_stop.cpp` Windows skip: same — variant suite under `#ifdef _WINDOWS`, or PORTABILITY.md note. Comment must explain *why* MHD's Windows path can't drive this scenario.
19+
- [x] For the postponed digest-auth Windows tests in `ws_start_stop.cpp:176-180` and `authentication.cpp:176-180`: either port them or document the upstream MHD-on-Windows limitation. If coordinated with TASK-062 (Digest RFC-7616), bring up there.
20+
- [x] For `ws_start_stop.cpp:337` `custom_socket` Darwin skip: add a comment naming the macOS-specific limitation, or port. Comment must reference the specific Darwin syscall behaviour that fails.
21+
- [x] Add a `scripts/check-skip-rationales.sh` lint that fails CI if a `#ifndef _WINDOWS` or `#ifndef DARWIN` block in `test/integ/` lacks a `// reason:` comment.
2222

2323
**Dependencies:**
2424
- Blocked by: None
@@ -34,4 +34,4 @@ Either land Windows/Darwin variants or document and gate the exclusion behind a
3434
**Related Requirements:** PRD §2 portability NFR
3535
**Related Decisions:** None new
3636

37-
**Status:** Backlog
37+
**Status:** Done

‎specs/tasks/M7-v2-cleanup/_index.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ TASK-093).
4141
| TASK-074 | Gate DEBUG raw-body printing behind explicit opt-in | LOW (sec) | S | Done |
4242
| TASK-075 | Propagate `wait_for_server_ready` to sibling hooks integ tests | HIGH | M | Done |
4343
| TASK-076 | Replace tautological-pass blocks in TLS test lanes | HIGH | M | Done |
44-
| TASK-077 | Restore Windows / Darwin coverage in skipped test suites | HIGH | L | Backlog |
44+
| TASK-077 | Restore Windows / Darwin coverage in skipped test suites | HIGH | L | Done |
4545
| TASK-078 | Resolve commented-out CONNECT-method test bodies | HIGH | S | Backlog |
4646
| TASK-079 | Drive nonce/opaque state machine in v2 digest-auth integ tests | MED | M | Backlog |
4747
| TASK-080 | Tighten threadsafety_stress latency gate back from 100× to 10× | MED | M | Backlog |

0 commit comments

Comments
 (0)