Nock-1077 Remove airplane mode #1209
Conversation
lib/interceptor.js
Outdated
| if (queries && _.isFunction(queries) && queries() === false) { | ||
| this.queries = {} | ||
| return this | ||
| } |
There was a problem hiding this comment.
Can you make a separate PR for the code change and only do tests? It's harder to review that way
There was a problem hiding this comment.
Sure!
Also, care to guess the expected behavior when function returns false in the query function? Just so I don't misinterpret the results I'm getting.
There was a problem hiding this comment.
I don’t know 🤷♀️ Can you open a separate issue/PR for discussion?
There was a problem hiding this comment.
Yeah. Once this test is fixed I'll work on it. :)
There was a problem hiding this comment.
@jlilja Any time to work on this? :)
tests/test_https_allowunmocked.js
Outdated
| request(options, (error, response, body) => { | ||
| test.true((error === null), 'should be no error') | ||
| test.true(typeof body !== 'undefined', 'body should not be undefined') | ||
| test.true(body.length !== 0, 'body should not be empty') |
There was a problem hiding this comment.
It looks like this is intended to fall through to the real server. Could you add assertions that match the body and/or status code to what the server returns?
|
@jlilja mind if we finish this up? It's the last bit to get rid of the AIRPLANE mode and it’s starting to block |
|
@gr2m do you want to wrap this one up? |
|
I’ll be busy until Jan 9th, I can look into it afterwards. Feel free to tackle it yourself if you like |
|
Terribly sorry, I've been away for holidays! If you haven't already taken over this PR, I will fix this today. @paulmelnikow have you started working on this? |
|
Please do! 😁 We're targeting the |
|
Sweet! Been using prettier since before, so no problem. Will the beta branch be a big merge with all of the tests updated? Or is this a branch that'll stay for future changes as well? |
|
We will eventually merge |
|
@jlilja How'd you make out? |
…t not refuse any other path than index.
|
I've made a fix for my code to do the check you asked about. However I couldn't make it work with the request package. I instead tried to do the same logic however with axios - and with axios it works just as I want. Now this probably raises another question as to why axios works and request fails. Depending on what you think I can probably merge it into beta or I'll go back to try and troubleshoot why it isn't working with request. |
|
I just saw that you're converting to the got package in other commits. If I switch from axios to got (and fix that linting travis notices), are we good? :) |
|
I've been applying linting fixes from the rules in the beta branch but it seems to still be complaining about my code in Travis, however not locally. Could someone have a look? |
|
One thing you could try is deleting Oh! I see that's the problem. Some updates to the lint config made in the |
tests/test_https_allowunmocked.js
Outdated
| const { body } = await got(`${url}/otherpath`) | ||
| t.true(JSON.parse(body).status === 'default') | ||
| } catch (error) { | ||
| console.warn(error) |
There was a problem hiding this comment.
I think these try/catch should be removed so those conditions will just throw. What do you think?
There was a problem hiding this comment.
That is probably better. Didn't think of the error handling while in a test.
tests/test_https_allowunmocked.js
Outdated
| console.warn(error) | ||
| } | ||
|
|
||
| t.end() |
There was a problem hiding this comment.
t.end() will happen automatically on tests with async.
There was a problem hiding this comment.
Didn't know that. I'll fix it.
tests/test_https_allowunmocked.js
Outdated
| nock('https://www.google.com/', { allowUnmocked: true }) | ||
| .get('/pathneverhit') | ||
| .reply(200, { foo: 'bar' }) | ||
| process.env['NODE_TLS_REJECT_UNAUTHORIZED'] = 0 |
There was a problem hiding this comment.
Can this be done with { rejectUnauthorized: true } passed to got instead?
There was a problem hiding this comment.
This i so confusing. While trying to change, the test appears to be working without the tls boolean. Could have been fixed I set the port explicitly to 3000 instead of letting https.createServer assign a random port. I'll push and see what Travis says as well.
There was a problem hiding this comment.
No. Stupid me. Was changing in the wrong repo and branch...
|
I'm still a bit confused if I've merged/rebased correctly. Let me know if I can do anything differently from here on. |
|
It looks good! In the future if you start with the |
|
OMG! Did we just hit our first big milestone?! A celebration is in order /cc @RichardLitt |
|
This is so exciting! This should be posted in #1268 😄 |
|
Woo! Well done, @jlilja! |
|
🎉 This PR is included in version 11.0.0-beta.4 🎉 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 📦🚀 |
Reference to #1077
Updated tests to use its own https server.
Query now also respects return value false. Why this happens during my rewrite I cannot understand or see. I think however passing functions in tests should be tested in file test_complex_querystrings.js.
Do we know the expected behavior of passing false in a function in query? If we do and it doesn't match the expected behavior of the second test, that should call for a new issue/pr.