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

test: fix assertRejects usage #1890

Merged
merged 2 commits into from Feb 10, 2020
Merged

Conversation

nikaspran
Copy link
Contributor

@nikaspran nikaspran commented Feb 10, 2020

As mentioned in #1889, it appears that assertRejects is not currently used correctly - the third parameter is the message that the assertion emits if it fails, not a matcher for the exception text.

This PR fixes all usages of assertRejects to correct expect on the error message. It might also be worth considering migrating to something like chai-as-promised for a more standard chai based approach.

Runkit with an example.

@@ -61,8 +60,7 @@ describe('`enableNetConnect()`', () => {

await assertRejects(
got('https://example.test/'),
Error,
'Nock: Disallowed net connect for "example.test:80/"'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good example of where the test was passing when it had a bug in it - the test expects port 80 when it was actually throwing an error for port 443 (https).

Copy link
Member

@paulmelnikow paulmelnikow left a comment

Choose a reason for hiding this comment

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

Great catch! I totally misread those docs. Thank you for fixing this.

@paulmelnikow
Copy link
Member

paulmelnikow commented Feb 10, 2020

By the way, would love to consider chai-as-promised here. See #1305 in which we’re migrating from Tap to Chai and Mocha. You can see linked some of the incremental PRs. It’s a big job and we’d love help with it if you’re interested! You can post in that issue to avoid duplicated effort.

My pref would be to finish converting to chai before we adopt chai-as-promised.

@nikaspran
Copy link
Contributor Author

@paulmelnikow Is anyone currently working on the tap-to-mocha convertion? Wouldn't want to step on any toes. If not, I could pick up some suites to convert.

Specifically talking about assertRejected, I imagine the plan would be something like this then:

  1. Finish tap -> mocha

  2. Adopt chai-as-promised and convert assertRejected into expectations, i.e.

    await expect(somePromise).to.eventually.be.rejectedWith(Error, /Nock: .../)

@paulmelnikow
Copy link
Member

paulmelnikow commented Feb 10, 2020

Best thing is to pick a file that’s still using tap assertions and start converting them. None have been claimed, except there’s one open PR for the Mocha DSL in test_logging.

If you post in #1305 when you start, then everyone else will know not to work on that one.

And yea, I think the order you’re suggesting makes sense.

@paulmelnikow paulmelnikow mentioned this pull request Feb 10, 2020
@paulmelnikow paulmelnikow merged commit 524dd29 into nock:master Feb 10, 2020
@github-actions
Copy link

🎉 This PR is included in version 11.9.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nikaspran nikaspran deleted the fix-assert-rejects branch February 11, 2020 06:04
@github-actions
Copy link

🎉 This PR is included in version 11.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants