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

chore: Replace catch with assert.rejects|throws in tests (part 3 of 3) #3661

Merged
merged 4 commits into from
May 7, 2021

Conversation

mdrichardson
Copy link
Contributor

@mdrichardson mdrichardson commented May 6, 2021

Fixes: #3426

Note: This is part 3 of 3, so the issue is done after this merges. There's some tests that weren't converted, for various reasons, detailed in each PR.

#minor

Description

We has some awkward usage of

try {
  methodThatShouldFail()
  assert.fail('not whatever')
} catch (err) {
	assert.strictEqual(err.message, 'whatever');
}

That I converted to

assert.throws(
	() => methodThatShouldFail(),
	Error('whatever')
);

Note

There's a commit with a bunch of no-op linting fixes. I recommend mostly reviewing the other one. However, I accidentally committed some no-op changes to skillDialog.test.js. sorry.

I skipped a LOT of tests in botframework-streaming because:

  1. They rely on chai, which doesn't handle rejects() without chai-as-promised
  2. The nature of the promises in this library don't work well with rejects() and are handled better/easier with the existing try/catch

@mdrichardson mdrichardson marked this pull request as ready for review May 6, 2021 22:31
@mdrichardson mdrichardson requested review from a team as code owners May 6, 2021 22:31
@mdrichardson mdrichardson requested review from tomlm and luhan2017 May 6, 2021 22:31
@coveralls
Copy link

coveralls commented May 6, 2021

Pull Request Test Coverage Report for Build 821233558

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.9%) to 85.441%

Totals Coverage Status
Change from base Build 820648573: 0.9%
Covered Lines: 19217
Relevant Lines: 21394

💛 - Coveralls

@stevengum stevengum merged commit a71af89 into microsoft:main May 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert catch in tests to use rejects()/throws(), fix assert.rejects()
3 participants