Skip to content

test: assert compression with Cache-Control lacking no-transform - #294

Open
renanmpimentel wants to merge 1 commit into
expressjs:masterfrom
renanmpimentel:test/cache-control-without-no-transform
Open

renanmpimentel wants to merge 1 commit into
expressjs:masterfrom
renanmpimentel:test/cache-control-without-no-transform

Conversation

@renanmpimentel

Copy link
Copy Markdown

What changed

Adds a test block next to the existing Cache-Control: no-transform tests asserting that a response is still compressed when it has a Cache-Control header that does not contain no-transform (no-cache and public, max-age=60).

Why

The Cache-Control tests only cover the negative side of shouldTransform(): every case sets a value containing no-transform and asserts that the response is not compressed. Nothing checks the positive side, so a regression that disables compression for any response carrying Cache-Control goes unnoticed.

For example, either of these changes to shouldTransform() in index.js leaves the whole suite green (63 passing):

-  return !cacheControl ||
+  return !cacheControl &&
     !cacheControlNoTransformRegExp.test(cacheControl)
-  return !cacheControl ||
-    !cacheControlNoTransformRegExp.test(cacheControl)
+  return !cacheControl

That would silently stop compressing a very common class of responses, including the Server-Sent Events example in the README, which sets Cache-Control: no-cache and relies on compression + res.flush().

Verification

  • npm test: 65 passing, 1 pending (was 63 passing, 1 pending)
  • npm run lint: no issues

Before / after against the regression above (|| → && in shouldTransform):

Suite Correct code With regression
Before this PR 63 passing 63 passing (regression undetected)
After this PR 65 passing 2 failing: expected "Content-Encoding" header field

The second variant (return !cacheControl) gives the same result.


I found this while experimenting with Supertest (unrelated to the supertest HTTP library), a tool for evaluating test effectiveness using mutation testing and test-harness mutilation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant