test+ci: qvalue-parser edge coverage + gcov coverage job#84
Merged
Conversation
Accept-Encoding qvalue parser (ngx_http_zstd_eval_qvalue / ngx_http_zstd_skip_quoted) had untested branches: repeated q param, q with no value, leading digit not 0/1, escaped quoted-pair inside a non-q param value, and an unterminated quoted-string running to end of field. Adds TAP TEST 80-84 covering each. New gcov/lcov coverage job builds an instrumented nginx, runs the full TAP+python suite, and enforces an 80% line-coverage floor (measured 80.2% after these tests, up from 79.2%) so future changes can't silently drop tested surface. Remaining gaps are mostly alloc-failure/ZSTD-API-error defensive paths not worth fault-injecting.
📝 WalkthroughWalkthroughThe pull request adds Accept-Encoding ChangesCoverage and negotiation validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant nginx
participant TestSuite
participant lcov
participant CoverageArtifact
GitHubActions->>nginx: build instrumented nginx with zstd module
GitHubActions->>TestSuite: run TAP and Python regression tests
TestSuite->>nginx: exercise zstd negotiation and module paths
GitHubActions->>lcov: capture and summarize line coverage
lcov->>GitHubActions: return coverage percentage
GitHubActions->>CoverageArtifact: upload coverage data and HTML report
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
01-static.t asserts a fixed ETag derived from t/suite/test{,.zst} mtime.
The tests/tests-asan jobs already pin it with touch -d before running;
the new coverage job forgot this step, so the checkout's fresh mtime
produced a different ETag and failed 45/321 static subtests on CI
(passed locally only because the local git checkout's mtime happened
to already be old). Same touch -d, same epoch, matching the existing
jobs.
![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
🤖 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/build-test.yml:
- Around line 946-947: Update the “Checkout module” actions/checkout step to
disable persisted checkout credentials by setting its persist-credentials option
to false, while preserving the existing pinned action version and other checkout
behavior.
- Around line 1003-1010: Update the “Run TAP suite” step to configure the
required servroot before invoking prove, matching the ASAN job’s setup so the
relative root ../../t/suite resolves to the t/suite fixtures. Preserve the
existing timestamp pinning and prove t/*.t execution.
In `@t/00-filter.t`:
- Around line 2025-2129: Replace the no_plan setup in the filter test suite with
the fixed repeat_each() * (blocks() * 3) + N assertion plan, recalculating N to
include tests 80–84 and match the suite’s total assertions.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f3b6ee04-b42a-4aa2-8070-0ff72e45b851
📒 Files selected for processing (2)
.github/workflows/build-test.ymlt/00-filter.t
Comment on lines
+946
to
+947
| - name: Checkout module | ||
| uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable persisted checkout credentials.
Repository-controlled tests run after checkout and can read the persisted token from Git configuration, which is especially risky on the self-hosted runner.
Proposed fix
- name: Checkout module
uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5
+ with:
+ persist-credentials: false📝 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
| - name: Checkout module | |
| uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5 | |
| - name: Checkout module | |
| uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 946-947: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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/build-test.yml around lines 946 - 947, Update the
“Checkout module” actions/checkout step to disable persisted checkout
credentials by setting its persist-credentials option to false, while preserving
the existing pinned action version and other checkout behavior.
Source: Linters/SAST tools
Comment on lines
+1003
to
+1010
| - name: Run TAP suite (drives filter/static/conf-warn paths) | ||
| run: | | ||
| export PERL5LIB="$HOME/perl5/lib/perl5:${PERL5LIB:-}" | ||
| cd "$GITHUB_WORKSPACE" | ||
| # 01-static.t asserts on a fixed ETag derived from fixture mtime; | ||
| # pin it or every run gets a fresh checkout mtime and fails. | ||
| touch -d @1541504307 "$GITHUB_WORKSPACE/t/suite/test" "$GITHUB_WORKSPACE/t/suite/test.zst" | ||
| prove t/*.t |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Configure the servroot required by the static TAP suite.
prove t/*.t includes t/01-static.t, but this step only pins timestamps. Arrange the servroot so its root ../../t/suite fixture path resolves, as the ASAN job already does.
Proposed fix
export PERL5LIB="$HOME/perl5/lib/perl5:${PERL5LIB:-}"
cd "$GITHUB_WORKSPACE"
+ export TEST_NGINX_SERVROOT="$GITHUB_WORKSPACE/t/servroot-static"
+ mkdir -p "$TEST_NGINX_SERVROOT"
# 01-static.t asserts on a fixed ETag derived from fixture mtime;As per coding guidelines, static-suite tests using relative roots must arrange the servroot so root ../../t/suite resolves correctly.
📝 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
| - name: Run TAP suite (drives filter/static/conf-warn paths) | |
| run: | | |
| export PERL5LIB="$HOME/perl5/lib/perl5:${PERL5LIB:-}" | |
| cd "$GITHUB_WORKSPACE" | |
| # 01-static.t asserts on a fixed ETag derived from fixture mtime; | |
| # pin it or every run gets a fresh checkout mtime and fails. | |
| touch -d @1541504307 "$GITHUB_WORKSPACE/t/suite/test" "$GITHUB_WORKSPACE/t/suite/test.zst" | |
| prove t/*.t | |
| - name: Run TAP suite (drives filter/static/conf-warn paths) | |
| run: | | |
| export PERL5LIB="$HOME/perl5/lib/perl5:${PERL5LIB:-}" | |
| cd "$GITHUB_WORKSPACE" | |
| export TEST_NGINX_SERVROOT="$GITHUB_WORKSPACE/t/servroot-static" | |
| mkdir -p "$TEST_NGINX_SERVROOT" | |
| # 01-static.t asserts on a fixed ETag derived from fixture mtime; | |
| # pin it or every run gets a fresh checkout mtime and fails. | |
| touch -d `@1541504307` "$GITHUB_WORKSPACE/t/suite/test" "$GITHUB_WORKSPACE/t/suite/test.zst" | |
| prove t/*.t |
🤖 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/build-test.yml around lines 1003 - 1010, Update the “Run
TAP suite” step to configure the required servroot before invoking prove,
matching the ASAN job’s setup so the relative root ../../t/suite resolves to the
t/suite fixtures. Preserve the existing timestamp pinning and prove t/*.t
execution.
Source: Coding guidelines
Comment on lines
+2025
to
+2129
| === TEST 80: repeated "q" parameter is malformed (RFC 9110 permits at most one) | ||
| --- config | ||
| location /filter { | ||
| zstd on; | ||
| zstd_types text/plain; | ||
| proxy_pass http://127.0.0.1:$TEST_NGINX_SERVER_PORT/test; | ||
| } | ||
| location /test { | ||
| root $TEST_NGINX_PERL_PATH/suite/; | ||
| } | ||
| --- request | ||
| GET /filter | ||
| --- more_headers | ||
| Accept-Encoding: zstd;q=0.5;q=0.9 | ||
| --- response_headers | ||
| Content-Length: 59738 | ||
| !Content-Encoding | ||
| --- no_error_log | ||
| [error] | ||
|
|
||
|
|
||
|
|
||
| === TEST 81: "q" parameter present without "=value" is malformed | ||
| --- config | ||
| location /filter { | ||
| zstd on; | ||
| zstd_types text/plain; | ||
| proxy_pass http://127.0.0.1:$TEST_NGINX_SERVER_PORT/test; | ||
| } | ||
| location /test { | ||
| root $TEST_NGINX_PERL_PATH/suite/; | ||
| } | ||
| --- request | ||
| GET /filter | ||
| --- more_headers | ||
| Accept-Encoding: zstd;q | ||
| --- response_headers | ||
| Content-Length: 59738 | ||
| !Content-Encoding | ||
| --- no_error_log | ||
| [error] | ||
|
|
||
|
|
||
|
|
||
| === TEST 82: qvalue leading digit not 0 or 1 is malformed (q=2) | ||
| --- config | ||
| location /filter { | ||
| zstd on; | ||
| zstd_types text/plain; | ||
| proxy_pass http://127.0.0.1:$TEST_NGINX_SERVER_PORT/test; | ||
| } | ||
| location /test { | ||
| root $TEST_NGINX_PERL_PATH/suite/; | ||
| } | ||
| --- request | ||
| GET /filter | ||
| --- more_headers | ||
| Accept-Encoding: zstd;q=2 | ||
| --- response_headers | ||
| Content-Length: 59738 | ||
| !Content-Encoding | ||
| --- no_error_log | ||
| [error] | ||
|
|
||
|
|
||
|
|
||
| === TEST 83: non-"q" parameter with an escaped quoted-pair does not confuse the delimiter scan | ||
| --- config | ||
| location /filter { | ||
| zstd on; | ||
| zstd_types text/plain; | ||
| proxy_pass http://127.0.0.1:$TEST_NGINX_SERVER_PORT/test; | ||
| } | ||
| location /test { | ||
| root $TEST_NGINX_PERL_PATH/suite/; | ||
| } | ||
| --- request | ||
| GET /filter | ||
| --- more_headers | ||
| Accept-Encoding: zstd;foo="a\"b,c";q=0.8, gzip;q=0.1 | ||
| --- response_headers_like | ||
| Content-Encoding: zstd | ||
| --- no_error_log | ||
| [error] | ||
|
|
||
|
|
||
|
|
||
| === TEST 84: unterminated quoted-string in a non-"q" parameter runs to end without OOB read, defaults q=1 | ||
| --- config | ||
| location /filter { | ||
| zstd on; | ||
| zstd_types text/plain; | ||
| proxy_pass http://127.0.0.1:$TEST_NGINX_SERVER_PORT/test; | ||
| } | ||
| location /test { | ||
| root $TEST_NGINX_PERL_PATH/suite/; | ||
| } | ||
| --- request | ||
| GET /filter | ||
| --- more_headers | ||
| Accept-Encoding: zstd;foo="unterminated | ||
| --- response_headers_like | ||
| Content-Encoding: zstd | ||
| --- no_error_log | ||
| [error] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Restore the suite’s fixed assertion plan.
Adding T80–84 while Line 28 remains plan 'no_plan' removes protection against premature test termination. Restore the required repeat_each() * (blocks() * 3) + N plan and recalculate N.
As per coding guidelines, t/00-filter.t: “Keep the filter test plan constant (plan tests => repeat_each() * (blocks() * 3) + N;) in sync with the actual number of assertions.”
🤖 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 `@t/00-filter.t` around lines 2025 - 2129, Replace the no_plan setup in the
filter test suite with the fixed repeat_each() * (blocks() * 3) + N assertion
plan, recalculating N to include tests 80–84 and match the suite’s total
assertions.
Source: Coding guidelines
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
q,qwith no value, leading digit not 0/1, escaped quoted-pair in a non-q param, unterminated quoted-string running to end of field.coverageCI job: gcov-instrumented nginx build, full TAP+python suite run, lcov capture, HTML report artifact, 80% line-coverage floor gate.Line coverage 79.2% -> 80.2%, branch coverage 68.1% -> 70.8% (function coverage already 100%). Remaining gaps are mostly alloc-failure/ZSTD-API-error defensive paths, not worth fault-injecting for marginal ROI.
Test plan
prove t/*.t(1322 tests) all passSummary by CodeRabbit
Bug Fixes
Accept-Encoding: zstdnegotiation parameters.Tests