test: expand 00-filter.t and 01-static.t coverage (+18 tests)#48
Merged
Conversation
Adds 18 new Test::Nginx::Socket cases targeting gaps surfaced by
auditing the directive table and the request paths against the
existing suite.
00-filter.t (TEST 47-60):
- 47/48 Vary: Accept-Encoding emitted with and without zstd negotiation
when gzip_vary is on.
- 49 zstd_buffers exercised with a small custom value.
- 50 zstd_target_cblock_size exercised on a sizeable input.
- 51 zstd_long on with zstd_window_log compresses cleanly.
- 52 zstd_min_length still applies on a chunked / unknown-Content-Length
upstream (the path TEST 6/7 do not cover).
- 53/54 comp_level boundary cases at 1 (min) and 22 (max).
- 55/56/57 Accept-Encoding parser: tab OWS around ';q=', case-insensitive
ZSTD coding name, stray empty list elements (,, zstd ,,).
- 58 subrequests never get zstd-encoded (auth_request).
- 59 zstd directive inside an if (...) block (NGX_HTTP_LIF_CONF).
- 60 filter bails out when upstream already set Content-Encoding.
01-static.t (TEST 22-25):
- 22 zstd_static always combined with gzip_vary on.
- 23 empty .zst declined (zero-byte magic-check edge).
- 24 directory-style request declines instead of serving.
- 25 gzip_vary on causes Vary: Accept-Encoding even when zstd_static
on declines for a non-accepting client (CDN cacheability).
Both files switch from the hand-maintained
"plan tests => repeat_each() * (blocks() * 3) + N" formula to
plan 'no_plan'. The fudge factor drifted whenever tests were added;
no_plan is the standard Test::Nginx idiom and removes the
maintenance trap going forward.
The handler correctly declines the empty .zst at the magic-check, but nginx's regular static handler then logs a benign ENOENT when looking for the uncompressed 'empty' fallback (which the fixture doesn't provide, by design — same as TEST 21). The 404 + !Content-Encoding assertions already lock the intended behaviour; the fallback ENOENT log is unavoidable and not what this test is about.
The comments documenting why Gap 2 (Accept-Ranges clearing) and Gap 3 (HTTP/2 transport) are deliberately not exercised by black-box tests were sitting between TEST 20 and TEST 21, separated from TEST 20's --- no_error_log section by blank lines. Test::Nginx::Socket parses everything from a --- section marker up to the next --- marker or === block as the section's content. The blank lines between [alert] and the # comments did not terminate the section, so every # line became an additional no_error_log pattern. At log_level 'debug' every nginx log entry contains 'PID#TID', so the '#' pattern alone matches everything and TEST 20 produced dozens of spurious 'not ok' lines. Master CI was masking this because the old hand-counted plan (blocks() * 3 + 63) was under-counting: TAP saw enough 'ok' lines to meet the plan and reported success. Switching to plan 'no_plan' in the previous commit made TAP count every assertion strictly, which surfaced the latent breakage. Move the notes above __DATA__ where they cannot be parsed into any test block, and leave TEST 20's no_error_log as just '[alert]'.
Two pre-existing latent issues that plan 'no_plan' surfaced: TEST 21 (zstd_static rejects a file whose contents are not a zstd frame) was registered against location /bogus_zst with root html, so the request /bogus_zst/bogus resolved to html/bogus_zst/bogus.zst. The --- user_files fixture creates files at <servroot>/html, not <servroot>/html/bogus_zst/, so open_cached_file hit ENOENT and the handler declined at the open step before ever reaching the magic check. The 'is not a zstd frame' error_log assertion therefore never fired. Change the location to / and the request to /bogus so the resolved path matches the fixture path; the magic-check is now actually exercised. master CI was masking this via a loose plan counter (blocks() * 3 + N) that was undercounting and absorbing the not-ok lines. TEST 22 originally asserted Vary: Accept-Encoding when zstd_static always is combined with gzip_vary on. That assertion was wrong: static.c only sets r->gzip_vary when enable == ON, never when enable == ALWAYS. In always mode the same .zst is served unconditionally so the response does not vary on Accept-Encoding; emitting Vary would mis-key shared caches. Flip the assertion to !Vary and rewrite the rationale to lock the intentional contract. Also apply the same /bogus_zst-style fixture-path fix to the new TEST 23 (empty .zst), which had inherited the same broken pattern from TEST 21.
Test::Nginx::Socket merges --- user_files into <servroot>/html/ and may auto-inject a default location / for serving those files. My previous fix used location / + GET /<name> for TEST 21 (fixed fixture path), TEST 23 (new), and TEST 24 (new) — three location / directives in tests that also use --- user_files collided with each other or with the auto-injected default, producing nginx [emerg] 'duplicate location /' and bailing the suite at TEST 23. Switch each to a unique non-root location prefix that matches the fixture's filename so root html still resolves correctly: TEST 21: location /bogus → /bogus → html/bogus.zst TEST 23: location /empty → /empty → html/empty.zst TEST 24: location /dir/ → /dir/ (directory-style, fixture-less) This also tightens TEST 24's docstring: the directive table guard at static.c uri.data[uri.len - 1] == '/' short-circuits before any filesystem work — that's the contract being locked.
A directory-style request that hits zstd_static's uri-suffix short-circuit then falls through to nginx's regular static handler, which 403s the directory and logs [error] when index.html is missing. That log entry is from core nginx, not from this module — exactly the post-decline behaviour the test wants to confirm. Expect error_code 403 (default index handler) and drop the no_error_log [error] assertion. The contract being locked is the same as before: !Content-Encoding (zstd_static did not falsely claim the directory response was zstd-encoded).
The fixture path ../../t/suite/dir/ does not exist, so nginx returns 404 (no such directory) rather than 403 (directory exists but index missing). The contract being locked is the same: !Content-Encoding, confirming zstd_static did not falsely claim the response was zstd-encoded.
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
Adds 18 Test::Nginx::Socket cases (14 filter, 4 static) targeting coverage gaps found by auditing the directive table and request paths against the existing suite. Also switches both files from the hand-maintained
plan tests => repeat_each() * (blocks() * 3) + Nformula toplan 'no_plan'— the fudge factor drifted every time tests were added, andno_planis the standard Test::Nginx idiom.What's covered
Filter (TEST 47–60)
Vary: Accept-Encodingemitted with and without zstd negotiation whengzip_vary onzstd_buffersexercised with a small custom valuezstd_target_cblock_sizeexercised on a sizeable inputzstd_long on+zstd_window_logcompresses cleanlyzstd_min_lengthon the chunked / unknown-Content-Length path (TEST 6/7 only cover known length)zstd_comp_levelboundaries 1 and 22;q=, case-insensitiveZSTD, stray empty list elements (,, zstd ,,)zstddirective inside anif (...)block (NGX_HTTP_LIF_CONF)Content-EncodingStatic (TEST 22–25)
zstd_static alwayscombined withgzip_vary on.zstdeclined (zero-byte magic-check edge)gzip_vary onstill emitsVary: Accept-Encodingwhenzstd_static ondeclines for a non-accepting client (CDN cacheability)Test plan