test: enhance req.is() tests with additional cases - #6703
Conversation
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
Signed-off-by: Sebastian Beltran <bjohansebas@gmail.com>
hamidrezaghavami
left a comment
hamidrezaghavami
left a comment
There was a problem hiding this comment.
your code was so structured, I mainly review it for learning purposes, well done!
| "lint": "eslint .", | ||
| "lint:fix": "eslint . --fix", | ||
| "test": "mocha --require test/support/env --reporter spec --check-leaks test/ test/acceptance/", | ||
| "test": "mocha --require test/support/env --exit --reporter spec --check-leaks test/ test/acceptance/", |
There was a problem hiding this comment.
Why is this --exit here? The tests should work (and work for me) without it. According to Mocha docs --exit is a workaround for improper cleanup and I don't see anything wrong here.
There was a problem hiding this comment.
Hmm, I don't even remember why I added that anymore, it's been quite a while since I opened this PR. But I think it was to make sure it would always close and wouldn't leave the process hanging
There was a problem hiding this comment.
There were PRs where the process would stay running indefinitely, consuming CI resources.
There was a problem hiding this comment.
CI uses test-ci:
express/.github/workflows/ci.yml
Lines 78 to 80 in ae6dd37
There have been no runs that have timed out in the last year. One run was cancelled after over an hour, but it seems to be a problem with one specific PR (#6742).
There was a problem hiding this comment.
Yep, and test-ci uses test. Maybe it was because of that PR, I honestly don't remember anymore.
3 participants
Basically, we support an array as an argument for req.is. This hasn’t been documented, and we should document it on the website.
Ref:
express/lib/request.js
Line 260 in 8f21493