pillarjs
test: skip query test on node <22
Follow-up to https://github.com/pillarjs/router/issues/162#issuecomment-3016193694
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
This mirrors the Express PR (https://github.com/expressjs/express/pull/6512) and skips testing the QUERY method on Node <22 in this repository too.
Could you do something like this in Express, please, so we don't have to repeat the same logic multiple times?
https://github.com/jonchurch/express/blob/bd302bf4606259a7703a06a2c5d269eed1679eb1/test/support/utils.js#L78
Could you do something like this in Express, please, so we don't have to repeat the same logic multiple times?
@bjohansebas done, put all the logic into a self-contained function. Let me know if your preference is to match the implementation in Express where the version string is provided by the caller and I can update
Now we just have to wait for a maintainer (TC) to approve the CI run, and then I’ll be able to give the green light.
Now we just have to wait for a maintainer (TC) to approve the CI run, and then I’ll be able to give the green light.
great! cc @UlisesGascon @wesleytodd
cc @UlisesGascon would be great to get this merged — anything I can do to help?
Something curious is that the error no longer appears, new PRs were opened and didn’t trigger any error in the CI
@bjohansebas oh, that's interesting
i just rebased my original PR https://github.com/pillarjs/router/pull/160 on the latest master here, could you approve my workflows and we can see if CI passes?
I also did not find any evidence of Node.js removing query either, yet the tests pass for node 20. Honestly it was long enough ago now that I am questioning if we ever saw this present in the Router tests. It is possible we only saw the test failures in express.
https://github.com/pillarjs/router/actions/runs/16882212279/job/47820666814?pr=177
Side note: please do not "ping" or "cc" me. I watch all the repo's I can and this makes it take longer to get through issues and buries important issues behind hundreds of noisy notifications. While it is great for folks to keep things moving forward this noise just makes a the problem worse. Thanks!
@wesleytodd great, thanks for the follow up. Looks like tests are passing all around without this PR. In that case, are we good to close this?