Improve tests of .reply() with a synchronous function #1521
Conversation
|
@mastermatt would you be up for reviewing this? All other reviewers welcome too :) |
|
The drop in coverage may be on account of the two tests removed for #1519. Let's make sure after that is merged that the coverage stays the same. |
| // The observed behavior is that this returns a 123 status code. | ||
| // | ||
| // The expected behavior is that this should either throw an error or reply | ||
| // with 200 and the JSON-stringified '[123]'. |
There was a problem hiding this comment.
I expected the observed behavior, given the current documentation.
Is there a plan to change the expected behavior with a breaking change?
There was a problem hiding this comment.
Hmm. It seems very confusing that the 123 status code would override an also-provided 200 status code without giving an error. It doesn't seem helpful either. It seems certain that someone who writes this does so by mistake.
Out of curiosity, is there something in the docs that makes it sound like it behaves this way? I'm game to fix it, breaking change or no. Maybe we can resolve this quickly enough to get it shipped in 11.0!
There was a problem hiding this comment.
Not from specific examples in the docs, but from an understanding that status, body, and headers can be provided two separate ways: either as separate args to reply or by returning an array from the callback passed to reply. My understanding (presumption?) was that if an array was returned from the callback, it would override other values.
But if that is not explicitly the desired design; I would vote for throwing an error if both were provided because it can be confusing, especially for newcomers to the lib. I've always thought the pattern of "if its an array, then it's a special value, otherwise it's the body" was a bit too magical.
There was a problem hiding this comment.
Huh, yea, my assumption was different. It would be good to include tests of all the cases and pick the behavior that seems safest and least confusing.
Let's take a look at the behavior in the context of the changes you're proposing in #1520. We can enable these tests and make sure we've got all the bases covered.
Other than the skipped tests, do these changes look good to you?
| .post('/endpoint', exampleRequestBody) | ||
| .reply(404, (uri, requestBody) => { | ||
| t.equal(uri, '/endpoint') | ||
| t.equal(requestBody, exampleRequestBody) |
There was a problem hiding this comment.
Is there anything that asserts these two lines get executed?
There was a problem hiding this comment.
Fortunately, yes: the t.plan(3) means exactly three tap asserts have to be called for the test to pass.
| test('reply can take a callback', async t => { | ||
| test( | ||
| 'reply function returning array with status code', | ||
| // Seems likely a bug related to https://github.com/nock/nock/issues/1222. |
There was a problem hiding this comment.
Is this not the desired behavior? #1170
There was a problem hiding this comment.
Yes, I think it is the desired behavior. However this test doesn't pass. The status code is 202 as expected, but the body is '[202]', while '' is expected.
|
|
||
| t.equal(statusCode, expectedStatusCode) |
There was a problem hiding this comment.
With the amount of hand waving that happens around optional data, it's probably worth explicitly asserting the status code and body down in these tests.
There was a problem hiding this comment.
Good shout. Updated!
|
Coveralls is reporting a coverage drop, though it looks exactly the same to me. I'm not inclined to worry about it. |
|
@gr2m any objections to my merging this based on @mastermatt's review? |
|
post-merge 😄👍 |
|
🎉 This PR is included in version 11.0.0-beta.12 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 11.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
- Add several tests for several missing edge cases. - Rewrite the test for #1222 so it reflects what I believe is the desired behavior. - Indicate one test where behavior is supported today but seems unnecessary. - Add one skipped test for a likely bug.
- Add several tests for several missing edge cases. - Rewrite the test for #1222 so it reflects what I believe is the desired behavior. - Indicate one test where behavior is supported today but seems unnecessary. - Add one skipped test for a likely bug.
- Add several tests for several missing edge cases. - Rewrite the test for #1222 so it reflects what I believe is the desired behavior. - Indicate one test where behavior is supported today but seems unnecessary. - Add one skipped test for a likely bug.
Depends on #1519 as it removes the two tests which are being moved elsewhere in that PR.