Conversation
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.
Hmm... I wonder why the DICOMWeb tests are showing up again? The tests for them are already merged:
Must be squash merge issue.
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.
Some small changes requested. See comments sprinkled throughout. Please re-request my review when changes are made, or if you have questions/comments.
Codecov Report
@@ Coverage Diff @@
## master #67 +/- ##
==========================================
+ Coverage 11.4% 12.71% +1.31%
==========================================
Files 154 154
Lines 4426 4434 +8
Branches 908 910 +2
==========================================
+ Hits 505 564 +59
+ Misses 3099 3060 -39
+ Partials 822 810 -12
Continue to review full report at Codecov.
|
beforeEach(() => { | ||
Math.random = jest.fn(() => 0.4677647565236618); | ||
}); | ||
Math.random = jest.fn(() => 0.4677647565236618); |
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.
Will this bleed into a different test suite? Do you need to "undo" this patch after all of these tests have run?
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.
Good point. It won't right now but it is safe to add a clearAllMocks
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.
@biharck, if you use Math.random somewhere else, I think it would still be a jest.fn? Doesn't clearAllMocks just reset the function so you can see what args are passed?
I think you need to do something like...
const savedFn = Math.random;
Math.random = jest.fn();
// after all
Math.random = savedFn;
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.
For example: jestjs/jest#7136 (comment)
But I think you're right that Math.random
calls in other test suites would use the baked in implementation, and not the one we override here.
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 PR is included in version 0.11.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.