Make (more) sure the error we get is the one we expect #267
Conversation
Since we set the statusCode to 500 in the changed test, a HTTPError is triggered whether the parsing throws or not. Because of this the test would still pass without the parse option set when it shouldn't. This change makes sure the error is the got.ParseError we expect.
|
Always happy to improve the tests. Thanks for the PR. I think it would be better to switch the whole test to use test('should have statusCode in err', async t => {
const err = await t.throws(got(`${s.url}/non200-invalid`, {json: true}), got.ParseError);
t.is(err.statusCode, 500);
}); |
|
@sindresorhus hi Sindre! |
Ava actually has the perfect function for this. Let's use it!
|
I had no idea Ava's Sidenote: That seems less than ideal. I'm not sure why that happens or how to improve it. I'll do some digging ^_^. |
Yes, no need, since we're passing in a promise, which is handled by
How are you breaking the test? |
test('should have statusCode in err', async t => {
const err = await t.throws(got(`${s.url}/non200-invalid`), got.ParseError);
t.is(err.statusCode, 500);
});I'm having a very hard time tracing the cause of the AssertionError Also cool hat ٩(◕‿◕｡)۶. |
|
@alextes Hmm. Weird. See: https://github.com/avajs/ava/blob/master/docs/recipes/debugging-with-chrome-devtools.md for debugging. |
|
@sindresorhus I know ava pretty well. She's amazing! Was already messing around with that but I couldn't seem to break T_T. Regardless I almost got it. No need for you to spend time on this. Go be awesome! function stdError(error, opts) {
if (error.code !== undefined) {
this.code = error.code;
}
Object.assign(this, {
message: error.message,
host: opts.host,
hostname: opts.hostname,
method: opts.method,
path: opts.path
});
}
got.ParseError = createErrorClass('ParseError', function (e, statusCode, opts, data) {
stdError.call(this, e, opts);
this.statusCode = statusCode;
this.statusMessage = http.STATUS_CODES[this.statusCode];
this.message = `${e.message} in "${urlLib.format(opts)}": \n${data.slice(0, 77)}...`;
}); |
|
I finally understand @sindresorhus 😓 . The check is still valid. After adding some defaults a Now to the silly bit. The reported error by ava is influenced by the caught error. In the case of breaking the test not to throw the expected Options:
For the third option, I would also have to add a commit that sets defaults for the I have a bit of a headache, but it was a good lesson! Hope to keep contributing 😄 . |
This ;) Could you open an issue (or PR) on AVA about this? |
|
Oh wow. Good debugging! :) |
Please do! I'm in https://gitter.im/sindresorhus/meta if you have any questions. Sorry about the delay. I was busy in real life. |
On it! I'll try and open a solid PR 😁.
ty ty 😊
Awesome! See you there.
No problem of course. I get you're busy. You're still getting the cat gifs I promised over in NodeJS slack 😸. |
Since we set the statusCode to 500 in the changed test, our code triggers a HTTPError. Regardless of the parsing conditional's outcome that may or may not throw. Because of this, the test would pass even if the code does not throw a ParseError. This change makes sure the error is the got.ParseError we expect. By no means perfect but good enough to save me and hopefully others from confusion.
It is a minor change, but it still tripped me up when working on #264. Hopefully, you too feel this would improve, not complicate the code 😄 .