Skip to content

test: fix assertRejects usage#1890

Merged
paulmelnikow merged 2 commits intonock:masterfrom
nikaspran:fix-assert-rejects
Feb 10, 2020
Merged

test: fix assertRejects usage#1890
paulmelnikow merged 2 commits intonock:masterfrom
nikaspran:fix-assert-rejects

Conversation

@nikaspran
Copy link
Copy Markdown
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.

await assertRejects(
got('https://example.test/'),
Error,
'Nock: Disallowed net connect for "example.test:80/"'
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

🎉 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
Copy Markdown

🎉 This PR is included in version 11.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mastermatt mastermatt added this to the Migrate to Mocha/Chai milestone Feb 18, 2020
juninmd pushed a commit to juninmd/nock that referenced this pull request Mar 21, 2026
As mentioned in nock#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.
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.

3 participants