Test failing on Node v20.x

[bjohansebas]

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

[nathanhleung]

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]

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]

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]

FWIW, we exclude the QUERY test in Node v21 (see #119 (comment)), 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.

router/test/route.js

Lines 257 to 259 in e6d6b60

	if (method === 'query' && process.version.startsWith('v21')) {
	return
	}

[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.

router/test/route.js

Lines 252 to 266 in e6d6b60

	methods.slice().sort().forEach(function (method) {
	if (method === 'connect') {
	// CONNECT is tricky and supertest doesn't support it
	return
	}
	if (method === 'query' && process.version.startsWith('v21')) {
	return
	}
	const body = method !== 'head'
	? shouldHaveBody(Buffer.from('hello, world'))
	: shouldNotHaveBody()
	describe('.' + method + '(...fn)', function () {
	it('should respond to a ' + method.toUpperCase() + ' request', function (done) {

[Nirmal1992]

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: forwardemail/supertest#860

[bjohansebas]

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

expressjs/express#6512

[nathanhleung]

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

#169

[efekrskl]

Should we close this? The tests are passing