-
Notifications
You must be signed in to change notification settings - Fork 98
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
Fabo/1379 remove errors in tests #1782
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1782 +/- ##
===========================================
- Coverage 95.07% 94.86% -0.21%
===========================================
Files 122 122
Lines 2982 2982
Branches 128 128
===========================================
- Hits 2835 2829 -6
- Misses 137 143 +6
Partials 10 10
|
Codecov Report
@@ Coverage Diff @@
## develop #1782 +/- ##
===========================================
- Coverage 95.07% 94.86% -0.21%
===========================================
Files 122 122
Lines 2982 2982
Branches 128 128
===========================================
- Hits 2835 2829 -6
- Misses 137 143 +6
Partials 10 10
|
That's Unit testing 👍 |
await TmSessionSignIn.methods.onSubmit.call(self) | ||
expect($store.commit).toHaveBeenCalledWith(`notifyError`, { | ||
title: `Signing In Failed`, | ||
body: expect.stringContaining(`Planned rejection`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks weird at first sight, but it's actually nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the direction towards more Unit oriented tests, if you have time we can change some let to const, otherwise let's merge as is 👍
@@ -69,16 +69,27 @@ describe(`TmSessionSignIn`, () => { | |||
}) | |||
|
|||
it(`should show a notification if signin failed`, async () => { | |||
store.dispatch = jest.fn(() => Promise.reject(`Planned rejection`)) | |||
wrapper.setData({ | |||
let $store = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: let
can be changed to const
if we don't plan to reassign those variables, there are some of those along the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's just turn this on https://eslint.org/docs/rules/prefer-const
Will merge and we can switch the rule on after deciding so in Tuesdays meeting. 👍 |
Closes #1379
Description:
❤️ Thank you!
CHANGELOG.md
with issue # and GitHub usernameFiles changed
in the github PR explorer