-
Notifications
You must be signed in to change notification settings - Fork 232
fix: only suppress console.error for non-pure imports #549
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #549 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 15 +1
Lines 210 218 +8
Branches 23 30 +7
=========================================
+ Hits 210 218 +8
Continue to review full report at Codecov.
|
I wasn't quite right about the docs and tests not needing updating. The docs were very close, but I have added more detail about the side effects, and the tests were still accurate, but there were some new cases that needed a test so I have refactored them a bit too. |
So would the way to disable the console patching to be to use the |
Using the pure import will disable it for the test file or setting the env variable (e.g. with the setup file utility) can disable it more globally. The docs cover these options. |
docs/api-reference.md
Outdated
@@ -12,6 +12,7 @@ route: '/reference/api' | |||
- [`cleanup`](/reference/api#cleanup) | |||
- [`addCleanup`](/reference/api#addcleanup) | |||
- [`removeCleanup`](/reference/api#removecleanup) | |||
- [`console.error`](/reference/api#consoleerror) |
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 reading what this list of links is for, I don't this this one should be added here after all (it will appear in the side menu still though).
Honestly, I'm feeling best about this option at the moment, especially now the suppression is being handled in before/after test hooks. |
Now that I've just said that I wonder if the "on import" of the previous change makes it slightly better for people wanting to mock Or moving to to ... Back to confusion. |
…can actually be registered
…uppression function
I've had a quick look at this, it does look promising. Resolving the |
Correct. No I think we should move forward with this one. It's not worse than what we have now, but also has the escape hatches if people need them. |
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.
Looks good, well done for exploring so many options, I don't think I would have been able to tackle this issue so thoroughly 😅
Ok... deep breath... here we go... |
🎉 This PR is included in version 5.0.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What:
Yet another alternative to #541 and #542
Why:
#546
How:
The complete opposite approach to #542 but the documentation and tests are still valid. By extending the suppression time from the initial import to after all tests have run, the patched
console.error
can be used by other utilities when setting up local mocks, while the code remains relatively straight forward and we don't need to wrapact
.Care here is taken to ensure that patching only occurs if the defaul imports are used and not the
/pure
imports.Checklist:
Fixes #546