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

Remove ESLint jest/valid-expect-in-promise #426

Merged
merged 6 commits into from
Apr 7, 2021

Conversation

astarinmymind
Copy link
Contributor

This PR refactors the way the tests catch errors. Currently they are wrapped in promises, but instead it can be refactored to use a jest trick of await expect(function).rejects.toThrow('error message'). Using this trick, we can remove this lint rule.

@astarinmymind astarinmymind requested a review from a team as a code owner March 23, 2021 03:44
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Mostly good! The Transaction controller tests need some adjustment

src/assets/AssetsController.test.ts Outdated Show resolved Hide resolved
src/transaction/TransactionController.test.ts Outdated Show resolved Hide resolved
src/transaction/TransactionController.test.ts Show resolved Hide resolved
Gudahtt
Gudahtt previously approved these changes Mar 25, 2021
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

Lots more examples left where an expect call is in an event handler that may or may not finish before the test ends, but we can fix that separately. I'll add another checklist item for that.

});
controller.cancelTransaction(controller.state.transactions[0].id);
result.catch((error) => {
expect(error.message).toContain('User rejected the transaction');
resolve('');
Copy link
Member

Choose a reason for hiding this comment

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

Nit: we don't need to pass anything to resolve here.

Suggested change
resolve('');
resolve();

Copy link
Member

Choose a reason for hiding this comment

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

Bah. I committed this, but now the test fails because the type needs to be adjusted. I'm going to undo this for now - we can fix it some other time.

@Gudahtt
Copy link
Member

Gudahtt commented Apr 7, 2021

I decided to go ahead and merge this now, to reduce the chance of future conflicts

@Gudahtt Gudahtt merged commit 8adc09b into develop Apr 7, 2021
@Gudahtt Gudahtt deleted the refactor-jest-async-await branch April 7, 2021 16:18
Gudahtt added a commit that referenced this pull request Apr 15, 2021
- Add restricted controller messenger ([#378](#378))

- **BREAKING:** Update minimum Node.js version to v12 ([#441](#441))
- **BREAKING:** Replace controller context ([#387](#387))
- Bump @metamask/contract-metadata from 1.23.0 to 1.24.0 ([#440](#440))
- Update lint rules ([#442](#442), [#426](#426))

- Don't remove collectibles during auto detection ([#439](#439))
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
* refactor catch errors to async await format

* finish promise callback removal

* restore ordering

* promise does not resolve

* use listener to ensure expects are called

Co-authored-by: Mark Stacey <markjstacey@gmail.com>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
* refactor catch errors to async await format

* finish promise callback removal

* restore ordering

* promise does not resolve

* use listener to ensure expects are called

Co-authored-by: Mark Stacey <markjstacey@gmail.com>
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.

2 participants