Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

skip QUERY method test #5628

Merged

Conversation

jonchurch
Copy link
Member

@jonchurch jonchurch commented Apr 29, 2024

related: #5615

First step is to stop running this test until there is a node version which supports QUERY properly.

resolves the failure we see in node 21 latest and node 22 latest

@jonchurch jonchurch force-pushed the jonchurch/fix-query-test-failure branch 2 times, most recently from 888246b to 438a1f5 Compare April 29, 2024 19:29
@wesleytodd
Copy link
Member

I wonder if we should do this in the methods package? Not sure how widely used that is outside of express but this could help in a more broad way if done there.

@joeyguerra
Copy link

Adding it to the methods module makes sense because it's an HTTP VERB.

@jonchurch
Copy link
Member Author

Good point.

From the readme of methods though, that doesn't sound like the contract that it claims to expose:

methods

This is an array of lower-cased method names that Node.js supports. If Node.js provides the http.METHODS export, then this is the same array lower-cased, otherwise it is a snapshot of the verbs from Node.js 0.10.

@ctcpip
Copy link
Member

ctcpip commented Apr 29, 2024

I have seen a lot of skipped tests in codebases that stay skipped long after they should have been fixed.

Can we do something preventative to avoid this happening in this case? For example, a check against process.version. The idea being that the TODO is inverted and becomes a task to re-commit to the skip when a new version is being tested (which importantly is forced via a test failure), rather than a TODO to remember to bring back/fix the test.

@wesleytodd
Copy link
Member

that Node.js supports

But it doesnt yet support it correctly, so I feel like that is alright.

@jonchurch
Copy link
Member Author

jonchurch commented May 2, 2024

Thats a good point though @ctcpip

How bout we skip it in 21 and 22, until we know what patch it should work in?

Edit: PR updated to do just that

The benefit to the ecosystem here is that this test failed node's citgm for express. We found a gap in Node between http.METHODS and actual support because it failed. We got that information, filed a bug, can act on it by skipping in affected releases, and are waiting for a patch.

But if 23 came out and it was broken, we'd def want to know that. Same for if it somehow could go red in any other past release.

We can skip with
// todo: enable test on 21 and 22 in supported versions once they land

Copy link
Member

@ctcpip ctcpip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome

@jonchurch jonchurch merged commit 8417c60 into expressjs:master May 4, 2024
26 checks passed
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.

None yet

4 participants