Skip to content

Conversation

@nathanhleung
Copy link

Follow-up to #162 (comment)

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

expressjs/express#6512

This mirrors the Express PR (expressjs/express#6512) and skips testing the QUERY method on Node <22 in this repository too.

@bjohansebas
Copy link
Member

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

@nathanhleung
Copy link
Author

Could you do something like this in Express, please, so we don't have to repeat the same logic multiple times?

jonchurch/express@bd302bf/test/support/utils.js#L78

@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

@bjohansebas
Copy link
Member

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.

@nathanhleung
Copy link
Author

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

@UlisesGascon UlisesGascon requested a review from wesleytodd June 30, 2025 05:34
@nathanhleung
Copy link
Author

cc @wesleytodd friendly ping

@nathanhleung
Copy link
Author

cc @UlisesGascon would be great to get this merged — anything I can do to help?

@bjohansebas
Copy link
Member

Something curious is that the error no longer appears, new PRs were opened and didn’t trigger any error in the CI

@nathanhleung
Copy link
Author

@bjohansebas oh, that's interesting

i just rebased my original PR #160 on the latest master here, could you approve my workflows and we can see if CI passes?

@wesleytodd
Copy link
Member

wesleytodd commented Aug 11, 2025

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

Screenshot 2025-08-11 at 8 57 49 AM

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!

@nathanhleung
Copy link
Author

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants