-
Notifications
You must be signed in to change notification settings - Fork 120
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
Fix restart button when Done #2610
Conversation
91ba393
to
b82ce16
Compare
Codecov Report
@@ Coverage Diff @@
## master #2610 +/- ##
==========================================
+ Coverage 76.20% 76.61% +0.41%
==========================================
Files 29 29
Lines 954 958 +4
==========================================
+ Hits 727 734 +7
+ Misses 227 224 -3
Continue to review full report at Codecov.
|
return core.prepare("a", true).then(() => { | ||
expect(core.readConfigFile).toHaveBeenCalledWith("a"); | ||
expect(core.readConfigFile).toHaveBeenCalledTimes(1); | ||
core.readConfigFile.mockRestore(); |
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.
You should restore the mocks in a .finally
to make sure you never influence other test cases
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.
Could you provide me a bit more guidance here please?
Is this different from the restore before each of the tests?
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.
should not be needed, but jest can be weird sometimes
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.
Ah, I was looking for the config option, didn't notice the manual beforeEach
- then you shouldn't need the restores at all
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.
yep
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.
Just to clarify, I'm happy to make changes here, but I'd still need some guidance on what's needed.
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.
looking pretty good, thank you!
return core.prepare("a", true).then(() => { | ||
expect(core.readConfigFile).toHaveBeenCalledWith("a"); | ||
expect(core.readConfigFile).toHaveBeenCalledTimes(1); | ||
core.readConfigFile.mockRestore(); |
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.
should not be needed, but jest can be weird sometimes
When the flashing process completes the user is asked if they want to flash another device. Currently the button seems to do nothing; these changes return the app back to the start screen to allow the user to attempt another flashing process. Co-authored-by: Johannah Sprinz <[email protected]>
Adds a unit test for the restart event.
Could you please |
When the flashing process completes the user is asked if they want to
flash another device. Currently the button seems to do nothing; these
changes return the app back to the start screen to allow the user to
attempt another flashing process.