Repository navigation
ci: scope the runtime driver, probe the Test::Nginx harness - #29
Conversation
The runtime driver had accreted cases the Perl suite covers, and the adoption prompt never checked the harness was installed before baselining against it. test_runtime.py: - test_reload_under_load waits for the master's 'start worker process' lines instead of sleeping either side of each SIGHUP: 5.01s -> 0.12s, and it now asserts the reload actually completed while traffic was in flight. The sleeping form never did, so on a loaded host it could pass while asserting nothing beyond the baseline case. - collect statuses per-thread under a lock; list.append across 8 threads is GIL-atomic today but not on the free-threaded build ci-deep.yml uses. - record why the seam cases cannot move to ci/t/: an arrayref raw_request is ONE request split into packets, not N requests, so a port of the every-interior-byte split passes green against the keep=0 mutation. PROMPT.md: - step 3 probes the .t count/location and whether Test::Nginx is present; step 5 records all three; step 6 blocks the baseline on them and demands the test count match. prove on a path with no .t files exits 0 with Result: NOTESTS, and step 10 is what moves t/ -> ci/t/, so a baseline taken beforehand is a green that ran nothing. - step 25 gains the driver-vs-ci/t boundary and an audit of what adopters added, with the move proved by a red in the new location. - note that Test::Nginx IS the openresty suite (AGENT/Test-Nginx-*.tar.gz); there is no separately-named package to look for. - fix a wrong cross-reference: the installer is ported at step 32, not 36. install-linters.sh: same openresty/Test::Nginx note at the cpan list.
WalkthroughThe PR expands test inventory and baseline validation requirements. It documents ChangesTest validation
Sequence Diagram(s)sequenceDiagram
participant RuntimeTest
participant RequestThreads
participant Nginx
RuntimeTest->>RequestThreads: Start concurrent requests
RuntimeTest->>Nginx: Send SIGHUP
Nginx-->>RuntimeTest: Report worker-start event
RequestThreads-->>RuntimeTest: Return request statuses
RuntimeTest->>Nginx: Send second SIGHUP
Nginx-->>RuntimeTest: Report second worker-start event
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 96ad0551-316d-4a45-af74-59f4060ea219
📒 Files selected for processing (3)
ci/PROMPT.mdci/linter/install-linters.shci/tools/test_runtime.py
| ls src/*_scan.c src/*_scan.h *_scan.c 2>/dev/null # decision seam? (step 7) | ||
| grep -ln 'ngx_http_request_t' src/*_scan.c *_scan.c 2>/dev/null # expect no hits | ||
| grep -nE '!\[' README.md # badge row + order | ||
| ls ci/t/*.t t/*.t tests/*.t 2>/dev/null | wc -l # how many .t, and WHERE? |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Record the .t location, not only the total.
Line 344 sends the matched paths to wc -l, so the probe discards the directory information. Lines 424-428 require both the count and the directory. The workflow at .github/workflows/build-test.yml Lines 311-329 runs prove ci/t/; an aggregate count can describe t/ or tests/ while the baseline runs a different suite. Emit a per-directory count or preserve the matched paths.
Suggested inventory probe
- ls ci/t/*.t t/*.t tests/*.t 2>/dev/null | wc -l
+ for dir in ci/t t tests; do
+ printf '%s: %s\n' "$dir" \
+ "$(find "$dir" -maxdepth 1 -type f -name '*.t' 2>/dev/null | wc -l)"
+ done📝 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.
| ls ci/t/*.t t/*.t tests/*.t 2>/dev/null | wc -l # how many .t, and WHERE? | |
| for dir in ci/t t tests; do | |
| printf '%s: %s\n' "$dir" \ | |
| "$(find "$dir" -maxdepth 1 -type f -name '*.t' 2>/dev/null | wc -l)" | |
| done |
| COUNT beside the verdict, check it against step 5's count, and treat `NOTESTS` as | ||
| "could not run", never as a pass — the same rule step 25's `-k` guard and | ||
| `run-all.sh`'s `LINT_ONLY` apply. A baseline of zero tests recorded as "green" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate ci/PROMPT.md =="
fd -a 'PROMPT\.md$' . || true
echo "== relevant ci/PROMPT.md lines =="
if [ -f ci/PROMPT.md ]; then
wc -l ci/PROMPT.md
sed -n '440,510p' ci/PROMPT.md | nl -ba -v440
fi
echo "== search for count/test inventory/prove/Files/Tests =="
rg -n "step 5|\\.t count|Files=|Tests=|recorded as|green|RUN|run-all|LINT_ONLY|NOTESTS" ci/PROMPT.md ci -S || true
echo "== list ci files =="
git ls-files ci | sed -n '1,120p'Repository: myguard-labs/nginx-skeleton-module
Length of output: 323
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect ci run scripts for inventory/counting =="
for f in $(git ls-files ci | rg '^(run|.*\.sh)$'); do
echo "--- $f ---"
wc -l "$f"
sed -n '1,260p' "$f" | nl -ba -v1
doneRepository: myguard-labs/nginx-skeleton-module
Length of output: 309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant ci/PROMPT.md lines =="
sed -n '440,510p' ci/PROMPT.md | awk '{printf "%7d\t%s\n", NR+439, $0}'
echo "== step references in ci/PROMPT.md =="
python3 - <<'PY'
from pathlib import Path
p=Path('ci/PROMPT.md')
for i,line in enumerate(p.read_text(encoding='utf-8').splitlines(),1):
if 'step 5' in line or 'step 25' in line or '.t count' in line or 'Tests=' in line or 'Files=' in line or 'NOTESTS' in line or 'LINT_ONLY' in line:
print(f"{i:6d}: {line}")
PY
echo "== ci script inventory candidates =="
find ci -maxdepth 2 -type f \) \( -name '*.sh' -o -path 'ci/*' \) -print 2>/dev/null | sort | awk '$1 ~ /\.sh$|PROMPT\.md$|README|Docker|compose|Makefile|run-all|inventory|test' { printf "%s\n", $0 }' | head -n 200Repository: myguard-labs/nginx-skeleton-module
Length of output: 6757
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all step 5 references =="
python3 - <<'PY'
from pathlib import Path
text = Path('ci/PROMPT.md').read_text(encoding='utf-8')
lines = text.splitlines()
for i, line in enumerate(lines, 1):
if 'step 5' in line:
print(f"{i}: {line}")
PY
echo "== ci file list =="
find ci -maxdepth 2 -type f 2>/dev/null | sort | sed -n '1,240p'
echo "== relevant run-all/linter files =="
for f in ci/run-all.sh ci/linter/run-all.sh; do
if [ -f "$f" ]; then
echo "--- $f ---"
sed -n '1,220p' "$f" | awk '{printf "%6d\t%s\n", NR, $0}'
fi
done
echo "== prove invocation/count-related wording in ci/PROMPT.md =="
python3 - <<'PY'
from pathlib import Path
lines = Path('ci/PROMPT.md').read_text(encoding='utf-8').splitlines()
for i, line in enumerate(lines, 1):
if any(s in line for s in ['prove', 'NOTESTS', 'File', 'Test', '.t count', 'Tests=', 'Files=', 'zero tests', 'number of tests']):
print(f"{i}: {line}")
PYRepository: myguard-labs/nginx-skeleton-module
Length of output: 13537
🌐 Web query:
perforce-prove-output Files= Tests= Test::Nginx assertions per test file "Files=" "Tests="
💡 Result:
The output pattern "Files= Tests=" is a standard summary format produced by the Perl Test::Harness library [1], which is the core engine behind the prove utility [2][1]. When prove finishes running a suite of tests, it outputs a summary line typically in the format: All tests successful. Files=N, Tests=M,... In this format: - Files=N indicates the total number of test files that were executed [1]. - Tests=M indicates the total count of individual test assertions (or sub-tests) run across those files [1]. Regarding the specific terms mentioned: - Test::Nginx: This is a Perl-based testing scaffold used for Nginx C modules and OpenResty-based applications [3][4]. It is designed to work with the standard Perl prove utility [2][5]. Because it leverages the underlying Perl testing infrastructure, it inherits the reporting format produced by prove and Test::Harness [2][1]. - Perforce: The search results indicate that Perforce (p4d) is often used in infrastructure testing environments (such as Git's own internal test suite, which includes git-p4 tests) [6][7]. In those contexts, prove may be used as the test runner [6][8]. The output format itself is not specific to Perforce; it is a general characteristic of the Perl testing ecosystem [1]. If you see this output in your environment, it confirms that your test suite is being executed by the Perl prove harness, regardless of whether you are testing Nginx modules or Perforce-related components [2][1].
Citations:
- 1: https://perldoc.perl.org/5.8.9/Test::Harness
- 2: https://openresty.gitbooks.io/programming-openresty/content/testing/running-tests.html
- 3: https://metacpan.org/pod/Test::Nginx::Socket
- 4: https://github.com/openresty/test-nginx/blob/master/lib/Test/Nginx.pm
- 5: https://nginx.googlesource.com/nginx-tests/+/fcfea55e5280f1590c41c8d38989738c00a7bfd8/README
- 6: https://code.googlesource.com/git/+/v2.9.0-rc2/t/lib-git-p4.sh
- 7: https://public-inbox.org/git/f9c68d4/s/?b=t%2Flib-git-p4.sh
- 8: http://public-inbox.org/git/03f29c1/s/?b=t%2Flib-git-p4.sh
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== step 5 content =="
sed -n '600,650p' ci/PROMPT.md | awk '{printf "%6d\t%s\n", NR+599, $0}'
echo "== .t inventory candidates =="
find ci -maxdepth 2 -type f 2>/dev/null | grep '/t/' | sort | awk '{printf "%s\n", $0}'
echo "== inventory/count exact phrases =="
python3 - <<'PY'
from pathlib import Path
text = Path('ci/PROMPT.md').read_text(encoding='utf-8').splitlines()
for i, line in enumerate(text, 1):
if any(s in line for s in ['inventory', '.t', 'Test::Nginx', 'count', 'list', 'ls']):
if i >= 560 and i <= 635 or i >= 150 and i <= 170 or i >= 600 and i <= 625:
print(f"{i}: {line}")
PYRepository: myguard-labs/nginx-skeleton-module
Length of output: 3208
Compare the .t file count with prove Files= results.
The suite inventory records .t files, but this wording calls the baseline check “tests” and says the result must match step 5. Since prove emits Files= and Tests= separately, compare the Files= count to the .t count and keep the assertion count separate.
| - **Stays in the driver** — many requests *in flight at once* (concurrency), and | ||
| anything driving a signal at the master while traffic runs (reload under | ||
| load). Test::Nginx is one request at a time; neither shape survives the move. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
perl -MTest::Nginx::Socket -e 'print "$Test::Nginx::Socket::VERSION\n"'
perldoc -T Test::Nginx::Socket | grep -nE 'multiple requests|pipelined_requests|raw_request'Repository: myguard-labs/nginx-skeleton-module
Length of output: 472
🏁 Script executed:
#!/bin/sh
set -eu
printf '--- ci/PROMPT.md context ---\n'
sed -n '1020,1050p' ci/PROMPT.md
printf '\n--- Test::Nginx references in repo ---\n'
rg -n "Test::Nginx|pipelined_requests|multiple requests|raw_request|concurrent" ci ci/PROMPT.md .github 2>/dev/null || trueRepository: myguard-labs/nginx-skeleton-module
Length of output: 263
🌐 Web query:
Test::Nginx::Socket pipelined_requests multiple requests raw_request documentation
💡 Result:
The Test::Nginx::Socket module provides the pipelined_requests directive to facilitate testing multiple HTTP requests over a single TCP connection, effectively simulating HTTP pipelining [1][2]. Key characteristics and usage guidelines include: 1. Data Structure: The pipelined_requests directive expects an array of request strings [3][2]. In practice, this is commonly implemented using the eval filter, which allows the use of Perl code to construct the array [2][4]. Example syntax: --- pipelined_requests eval ["GET /t", "GET /t"] 2. Sequential Execution: All requests specified in the array are sent over the same keep-alive connection, and Nginx processes them sequentially [3][5]. 3. Limitations and Compatibility: - Raw Request Interaction: The pipelined_requests and raw_request features are distinct and generally cannot be used simultaneously within the same test block [1][6]. - Protocol Restrictions: These features are designed for HTTP/1 protocols [1]. When using advanced test modes such as HTTP/2 or HTTP/3, the pipelined_requests and raw_request sections will still default to using HTTP/1 protocols [1][6]. - Response Verification: When using pipelined_requests, it is often necessary to use the eval filter for corresponding response sections (like response_body) to verify individual responses for each request in the pipeline [2][4]. For further implementation details, the Test::Nginx::Socket documentation on MetaCPAN provides comprehensive guidance on using these features within Nginx and OpenResty test suites [1][7].
Citations:
- 1: https://metacpan.org/pod/Test::Nginx::Socket
- 2: https://openresty.gitbooks.io/programming-openresty/content/testing/preparing-tests.html
- 3: https://github.com/openresty/test-nginx/blob/master/lib/Test/Nginx/Socket.pm
- 4: https://github.com/openresty/programming-openresty/blob/master/testing/preparing-tests.adoc
- 5: http://www.nginx-discovery.com/2011/03/day-33-testnginx-pipelinedrequests.html
- 6: https://www.gsp.com/cgi-bin/man.cgi?section=3&topic=Test%3A%3ANginx%3A%3ASocket
- 7: https://metacpan.org/pod/Test::Nginx
🏁 Script executed:
#!/bin/sh
set -eu
printf '--- ci/PROMPT.md context ---\n'
sed -n '1020,1050p' ci/PROMPT.md
printf '\n--- Test::Nginx references in repo ---\n'
rg -n "Test::Nginx|pipelined_requests|multiple requests|raw_request|concurrent" ci ci/PROMPT.md .github 2>/dev/null || trueRepository: myguard-labs/nginx-skeleton-module
Length of output: 263
🏁 Script executed:
#!/bin/sh
set -eu
echo '--- ci/PROMPT.md context ---'
sed -n '1020,1050p' ci/PROMPT.md
echo
echo '--- Test::Nginx references in repo ---'
rg -n "Test::Nginx|pipelined_requests|multiple requests|raw_request|concurrent" ci ci/PROMPT.md .github 2>/dev/null || true
echo
echo '--- Read-only repo check ---'
if [ -f ci/PROMPT.md ]; then
echo "ci/PROMPT.md exists"
else
echo "ci/PROMPT.md missing"
fiRepository: myguard-labs/nginx-skeleton-module
Length of output: 11146
Describe the unsupported request shape precisely.
This wording excludes valid Test::Nginx multi-request patterns. The live-server cases cannot express concurrent requests on independent connections or signal delivery while traffic runs, while Test::Nginx can run sequential or pipelined requests on one connection.
Suggested wording
- Test::Nginx is one request at a time; neither shape survives the move.
+ Test::Nginx can issue multiple requests, but it cannot express concurrent
+ requests on independent connections while the master is reloading; neither
+ shape survives the move.📝 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.
| - **Stays in the driver** — many requests *in flight at once* (concurrency), and | |
| anything driving a signal at the master while traffic runs (reload under | |
| load). Test::Nginx is one request at a time; neither shape survives the move. | |
| - **Stays in the driver** — many requests *in flight at once* (concurrency), and | |
| anything driving a signal at the master while traffic runs (reload under | |
| load). Test::Nginx can issue multiple requests, but it cannot express concurrent | |
| requests on independent connections while the master is reloading; neither | |
| shape survives the move. |
| with ThreadPoolExecutor(max_workers=8) as pool: | ||
| futures = [pool.submit(hammer) for _ in range(8)] | ||
| time.sleep(1.0) | ||
| srv.reload() | ||
| time.sleep(1.0) | ||
| srv.reload() | ||
| for f in futures: | ||
| f.result() | ||
| try: | ||
| for n in (1, 2): | ||
| # Requests are already in flight (the pool is running), so the | ||
| # SIGHUP lands under load, which is the whole point of R3. | ||
| srv.reload() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Establish request overlap before each reload.
pool.submit() does not ensure that any hammer() task has reached get() before srv.reload() runs. The main thread can send one or both SIGHUP signals before traffic starts. The final seen check can then pass from requests that complete after the reload window.
Record request intervals and require at least one interval to overlap each SIGHUP-to-worker-start interval before accepting that reload cycle. This makes the stated under-load precondition observable.
| raise Failure( | ||
| f"reload {n} did not bring up new workers within 10s " | ||
| f"(saw {worker_starts() - base} starts, want {2 * n}); " | ||
| "the reload never completed, so this case would have " | ||
| "asserted nothing" | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the reported Ruff TRY003 violation.
Ruff reports TRY003 for the long inline Failure message. If this rule is enabled in CI, the claimed clean lint baseline cannot hold. Move stable text into a helper or apply a narrow suppression if this diagnostic must remain inline.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 418-423: Avoid specifying long messages outside the exception class
(TRY003)
Source: Linters/SAST tools
1 participant
TL;DR
A test that reloads the server was sleeping for five seconds on every push to check something it could watch for directly, and it never confirmed the reload had actually happened while requests were being served. It now waits for the event: same assertion, five seconds down to a tenth of one, and it catches a failure the old version could sleep straight through.
Separately, the adoption prompt told adopters to run the test suite and record the result about thirty steps before anything guaranteed the test framework was installed — and the test runner reports success when it finds no test files at all, so a "green" baseline could mean nothing ran.
What changed
ci/tools/test_runtime.pytest_reload_under_loadwaited 1.0s, sentSIGHUP, waited 1.0s, sent another, and bounded the hammer threads with a 5s deadline. Two problems: the 5s is paid on every push, and nothing checked a reload had completed while requests were in flight. On a loaded build host a SIGHUP still queued when the hammer stopped left the case asserting only "the server answered 403" — whichtest_marker_is_blockedalready proves.It now counts the master's
start worker processlines and proceeds once the expected number appear, failing explicitly if they do not arrive within 10s. The reload becomes a precondition of the assertion instead of a hope.Status collection moved to a per-thread list merged under a lock.
list.appendfrom 8 threads is atomic under the GIL today, butci-deep.ymlalso runs the free-threaded build, where it is not.ci/PROMPT.md.tcount and location, and whetherTest::Nginximports at all. Step 5 records all three. Step 6 blocks the baseline on them and requires the test count to match.ci/t/, plus an audit of what adopters added to the driver — with the move proved by a red in the new location, not a green.Test::Nginxis the openresty suite (AGENT/Test-Nginx-*.tar.gz, AGENT being agentzh). There is no separately-named package, and nothing in the repo said so.ci/linter/install-linters.sh— the same openresty/Test::Nginxnote beside the cpan list.Why the seam cases did not move
--- raw_requestwith an arrayref looks like it expresses "N requests, one connection each". It does not:Test::Nginx::Socket::get_req_from_block(Socket.pm:337-347) turns an arrayref into one request split into packets. A port of the every-interior-byte split therefore sent one giant request, matched the first status line, and reported 2 assertions for what should have been 18 and 23 splits.It passed against the
keep = 0seam mutation that the Python driver catches. Written up in the driver docstring and step 25 so the next person does not spend the afternoon rediscovering it.--- pipelined_requestsis not the fallback either — one shared connection, so no case in the batch can carryConnection: close.Testing
Result: PASS.test_reload_under_load5.01s -> 0.12s; total ~6s -> ~1.1s.skel off(via aServer.reloadoverride — no one-line source edit produces this defect). Caught:165/349 requests stopped being blocked across a reload. The faster case is not a weaker one.NOTESTSclaim verified, not assumed:proveon an empty directory printsResult: NOTESTSand exits 0.ci/linter/run-all.sh:== all linters clean ==. Branch roster pre-pass: the shellcheck and markdownlint findings reproduce onmainwithout this branch; the two MD013 hits this diff introduced are fixed.test_reload_under_loadstill has no cheap source mutation — the defect it hunts is a conf-struct lifetime bug across SIGHUP. The control above proves it detects a lost verdict, not that specific bug. Stated in the docstring rather than glossed.