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

Return 500 instead of 404 #2558

Open
2 tasks done
simlu opened this issue Nov 23, 2023 · 4 comments
Open
2 tasks done

Return 500 instead of 404 #2558

simlu opened this issue Nov 23, 2023 · 4 comments

Comments

@simlu
Copy link

simlu commented Nov 23, 2023

Please avoid duplicates

Reproducible test case

n/a

Nock Version

nock@13.3.8

Node Version

node 18

TypeScript Version

No response

What happened?

When a nock is not found, a 500 should be returned, not a 404.

It's really confusing that the library returns with a 404. Yes, the nock is not found, but that's not what the request is interested in from the requesters perspective. A 404 indicates that the page that was requested was not found, not that there was an internal problem with the testing library.

Proposed change: Change 404 to 500 in this case.

Yes, I'd be happy to create a pr for this.

Code reference is here:

err.statusCode = err.status = 404

Would you be interested in contributing a fix?

  • yes
@simlu simlu added the bug label Nov 23, 2023
@simlu
Copy link
Author

simlu commented Nov 24, 2023

Workaround is done here: https://github.com/blackflux/node-tdd/pull/1404/files

@gr2m
Copy link
Member

gr2m commented Nov 27, 2023

I can see both sides. Given that we only did 404, changing it now might come at a high cost of friction when upgrading to the breaking version we introduce this change in. Or it might make people aware they had false positives 😬 It's also the first time this request came up as far as I know, at least since I maintain @nock (5+ years now). I suggest we let it sit for a while and see what other folks think

@simlu
Copy link
Author

simlu commented Nov 27, 2023

@gr2m I suspect that this hasn't come up because this functionality isn't widely used. Would be interesting to know how often this gets hit.

Is this an error that should ever come up in normal usage? My understanding is that this would only come up if something went really, really wrong with setting up a test / recording?

If that is true it really should be a 500. And yes, it might break tests, but only those tests that were bad in the first place.


[...] Or it might make people aware they had false positives 😬 [...]

Well, it would be great to let folks know that their tests are bad

I can see both sides. [...]

Honestly, I had trouble understanding why a status code was returned here at all. But I could see this if the test framework runs on a remote server and the client needs debug information. However, as to returning a 404 - I can not see a reason. The client makes a request for a resource on a server. The server returns a 404 - this means that the resource was not found. NOT that the server had an internal error with a testing library that it is using.

Why do you think it would be reasonable to return a 404 here? Am I misunderstanding how this error is used in nock?


tldr; We patched over this in node-tdd, so this is not pressing for us. But returning a 5xx is the correct way to go IMO

@gr2m
Copy link
Member

gr2m commented Nov 27, 2023

Why do you think it would be reasonable to return a 404 here

When thinking of nock as a backend, and a route could not be matched with a handler, 404 seems like a valid response code. However I was not part of the project when that decision was made.

If I'd implement this today, I'd probably vote go with a 501 https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/501.

The situation we are in now is that it was 404 for a long time, changing it to a different response code is a breaking change. It always comes with a friction. I agree shouldn't be much of a friction in this case though.

Unless there are any objections, I'd say we change 404 to 501 with the next breaking change of nock

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

No branches or pull requests

3 participants