router icon indicating copy to clipboard operation
router copied to clipboard

Test failing on Node v20.x

Open bjohansebas opened this issue 7 months ago • 8 comments

If someone could help us resolve this, that would be great. (ref: https://github.com/pillarjs/router/actions/runs/15097652965/job/42434464916?pr=161)

bjohansebas avatar May 18 '25 20:05 bjohansebas

Investigating — it seems like something changed in the latest Node 20 release which came out last week

nvm install 20.19.1
npm run test

[...]

678 passing (1s)
nvm install 20.19.2
npm run test

[...]

214 passing (374ms)
  1 failing

  1) Router
       .route(path)
         .query(...fn)
           should respond to a QUERY request:
     Error: expected 200 "OK", got 404 "Not Found"
      at Context.<anonymous> (test/route.js:274:14)

nathanhleung avatar May 21 '25 16:05 nathanhleung

This is interesting — I made a file test.js:

const http = require("http");

const server = http.createServer((req, res) => {
  res.writeHead(200, { "Content-Type": "text/plain" });
  res.end("Hello World\n");
});

server.listen(3000, async () => {
  console.log("Server running at http://localhost:3000/");

  const res = await fetch("http://localhost:3000/", {
    method: "QUERY",
  });
  console.log(res.status);
});

I get different output on Node v20.19.1 vs v20.19.2:

nvm use 20.19.1
node test

Server running at http://localhost:3000/
400
nvm use 20.19.2
node test

Server running at http://localhost:3000/
200

nathanhleung avatar May 21 '25 17:05 nathanhleung

More testing in test.js:

const { METHODS } = require("http");

console.log(METHODS);

Note that QUERY is added in v20.19.2:

nvm use 20.19.1
node test

[
  'ACL',         'BIND',       'CHECKOUT',
  'CONNECT',     'COPY',       'DELETE',
  'GET',         'HEAD',       'LINK',
  'LOCK',        'M-SEARCH',   'MERGE',
  'MKACTIVITY',  'MKCALENDAR', 'MKCOL',
  'MOVE',        'NOTIFY',     'OPTIONS',
  'PATCH',       'POST',       'PROPFIND',
  'PROPPATCH',   'PURGE',      'PUT',
  'REBIND',      'REPORT',     'SEARCH',
  'SOURCE',      'SUBSCRIBE',  'TRACE',
  'UNBIND',      'UNLINK',     'UNLOCK',
  'UNSUBSCRIBE'
]
nvm use 20.19.2
node test

[
  'ACL',        'BIND',        'CHECKOUT',
  'CONNECT',    'COPY',        'DELETE',
  'GET',        'HEAD',        'LINK',
  'LOCK',       'M-SEARCH',    'MERGE',
  'MKACTIVITY', 'MKCALENDAR',  'MKCOL',
  'MOVE',       'NOTIFY',      'OPTIONS',
  'PATCH',      'POST',        'PROPFIND',
  'PROPPATCH',  'PURGE',       'PUT',
  'QUERY',      'REBIND',      'REPORT',
  'SEARCH',     'SOURCE',      'SUBSCRIBE',
  'TRACE',      'UNBIND',      'UNLINK',
  'UNLOCK',     'UNSUBSCRIBE'
]

nathanhleung avatar May 21 '25 17:05 nathanhleung

FWIW, we exclude the QUERY test in Node v21 (see https://github.com/pillarjs/router/pull/119#issuecomment-2323438421), so one remedy could be to do the same here and exclude the test on Node v20 as well. cc @wesleytodd in case you have more context on why exactly we're skipping on 21.

https://github.com/pillarjs/router/blob/e6d6b609fc355e558174ccd5b1db646f739fe88c/test/route.js#L257-L259

nathanhleung avatar May 21 '25 17:05 nathanhleung

Note that the addition of QUERY to the http.METHODS constant in v20.19.2 effectively adds a test to the test suite, since tests are generated by iterating over the http.METHODS array.

That means that omitting the QUERY test would actually run the same tests as were run in v20.19.1. On v20.19.2, there are extra tests that weren't present before because there's the new element "QUERY" in the METHODS array. It seems like these extra tests are what are causing the breakage.

https://github.com/pillarjs/router/blob/e6d6b609fc355e558174ccd5b1db646f739fe88c/test/route.js#L252-L266

nathanhleung avatar May 21 '25 17:05 nathanhleung

Seems to be an issue with supertest dependency.. Since we use it for making API calls, and it does not support the latest query method, the test fails with a 404 error. This bug is already reported on supertest repo: Ref: https://github.com/ladjs/supertest/issues/860

Nirmal1992 avatar Jun 01 '25 08:06 Nirmal1992

This is due to the latest Node.js security patch, it's probably enough to just skip that test

https://github.com/expressjs/express/pull/6512

bjohansebas avatar Jun 29 '25 00:06 bjohansebas

@bjohansebas cool, just opened a PR which skips those tests on Node <22

https://github.com/pillarjs/router/pull/169

nathanhleung avatar Jun 29 '25 01:06 nathanhleung

Should we close this? The tests are passing

efekrskl avatar Nov 11 '25 18:11 efekrskl