express
express copied to clipboard
expressjs
Solve issues for Node.js CITGM
Hi team! I was researching a bit about the current issues with CITGM, there is a PR https://github.com/nodejs/citgm/pull/977 open to re-add express, but one of the tests is failling.
How to reproduce?
- Use Node 20.x (recommended 20.11.x)
nvm use 20.11.0 - Install cigtm globally
npm i -g citgm - Run the tests for express
citgm express
What is the error?
info: starting | express
info: lookup | express
info: lookup-notfound | express
info: lookup-githead | https://github.com/expressjs/express/archive/8368dc178af16b91b576c4c1d135f701a0007e5d.tar.gz
info: express npm: | Downloading project: https://github.com/expressjs/express/archive/8368dc178af16b91b576c4c1d135f701a0007
info: express npm: | Project downloaded 8368dc178af16b91b576c4c1d135f701a0007e5d.tar.gz
info: express npm: | npm install started
info: express npm: | npm install successfully completed
info: express npm: | test suite started
error: failure | The canary is dead:
error: failing module(s) |
error: module name: | express
error: version: | 4.18.2
error: error: | The canary is dead:
error: error: | undefined
error: | added 432 packages in 36s
error: |
error: | > [email protected] test
error: | > mocha --require test/support/env --reporter spec --bail --check-leaks test/ test/acceptance/
error: |
error: |
[...redacted...]
error: | 350 passing (636ms)
error: | 1 failing
error: |
error: | 1) express.json()
error: | with strict option
error: | when undefined
error: | should 400 on primitives:
error: | Error: expected `[entity.parse.failed] Unexpected token 't', "#rue" is not valid JSON` response body, got `[entity.parse.failed] Unexpected token 't', "#" is not v
error: | at Context.<anonymous> (test/express.json.js:265:12)
error: | at process.processImmediate (node:internal/timers:478:21)
error: | ----
error: | at error (node_modules/supertest/lib/test.js:335:15)
error: | at Test._assertBody (node_modules/supertest/lib/test.js:206:16)
error: | at /private/var/folders/7h/550p8fxd2gn4vwn6x_q9stdh0000gn/T/421860ca-ea70-4620-92e0-1d11e288e294/express/node_modules/supertest/lib/test.js:308:13
error: | at Test._assertFunction (node_modules/supertest/lib/test.js:285:13)
error: | at Test.assert (node_modules/supertest/lib/test.js:164:23)
error: | at Server.localAssert (node_modules/supertest/lib/test.js:120:14)
error: | at Object.onceWrapper (node:events:632:28)
error: | at Server.emit (node:events:518:28)
error: | at emitCloseNT (node:net:2279:8)
error: | at process.processTicksAndRejections (node:internal/process/task_queues:81:21)
error: |
error: |
error: |
error: |
error: | npm notice
error: | npm notice New minor version of npm available! 10.2.4 -> 10.4.0
error: | npm notice Changelog: <https://github.com/npm/cli/releases/tag/v10.4.0>
error: | npm notice Run `npm install -g [email protected]` to update!
error: | npm notice
error: done | The smoke test has failed.
info: duration | test duration: 49198ms
I was not able to reproduce this error while running the tests locally from a express clone, but after doing a bit of research seems like it is related to https://github.com/expressjs/body-parser/commit/368a93a613a1ac6cbdec9694f4018e707b3c1f50 ?
As a side note, I will update the CI to include the most recent Node.js versions:
- [ ] https://github.com/expressjs/express/pull/5430
- [ ] https://github.com/expressjs/express/pull/5429
- [x] https://github.com/expressjs/express/pull/5490
Yes, I fixed that a long time ago, also in this repo too, which is probably why you cannot reproduce it while running tests locally from clone?
Great point @dougwilson. Now I am thinking that maybe is because CITGM is using 4.18.2 and the patch in [email protected] is not yet released within express? 🤔
unreleased
==========
* Fix routing requests without method
* deps: [email protected]
- Fix strict json error message on Node.js 19+
- deps: content-type@~1.0.5
- deps: [email protected]
4.18.2 / 2022-10-08
===================
* Fix regression routing a large stack in a single route
* deps: [email protected]
- deps: [email protected]
- perf: remove unnecessary object clone
* deps: [email protected]
Oh, interesting. I guess that is true, if citgm uses published vereion it wouldn't be there. I was confused bc I saw the log saying downloading from github.com. I guess we can queue up a parch release quickly to make it available to citgm, without going through the minor release timeline.
Could we maybe do a release as a group on video? One of the things we're gonna need to get better at is training people and how to do these things safely and with the correct procedure. This seems like a good one to get started with.
Oh, ok, good thing you mentioned! I wqs gonna try and squeeze it in today so we could get the citgm checked off, but I can hold off for us to find a time and day to do it together.
Obviously we want to land the 5 release soonish, but other than that do we have anything pending to use as an example to record for educational purposes?
This was just to make a patch for 4 for citgm to use. Happy to do whatever. I have to leave in about 45 mins, and was on to do this patch before the work week starts and I get more limited, but happy to di whatever we want, just let me know. I'll keep holding until I hear more.
To circle back, there was not time today to finish the CICD and also record this release. We will try and get a recording in a future release, so going to mark my comments and the rest as off topic. Sorry for delaying it.
Curent status
We are just waiting for this PR to be merged, the CI is passing 🥳
