-
Notifications
You must be signed in to change notification settings - Fork 142
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 test cleanup tasks order #3141
Conversation
EventTarget.prototype.addEventListener = jasmine | ||
.createSpy() |
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 seems unrelated to this PR, but in any case:
❓ question: spyOn(EventTarget.prototype, 'addEventListener')
didn't work?
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 found cases where mocking the same API both manually with registerCleanup
and with spyOn
case don't work. I can't really explain why, but it seems reasonable to use the same mocking strategy twice. wdyt?
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.
wdyt?
I guess spies are removed after all afterEach
, so we have an order issue similar to what you are fixing in this ticket. So, I guess it makes sense.
Your explanation makes sense but it's a bit annoying that we cannot use Jasmine tools for that. This sounds extremely error prone..
One workaround I have in mind is to always attach spies first, before any other instrumentation. Then they will be detached after everything, so it will be fine.
An alternative could be to have our own spyOn
utility that works with registerCleanupTask
and forbid the jasmine one.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3141 +/- ##
==========================================
- Coverage 93.71% 93.44% -0.28%
==========================================
Files 279 279
Lines 7682 7683 +1
Branches 1718 1718
==========================================
- Hits 7199 7179 -20
- Misses 483 504 +21 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
Motivation
We are transitionning from using
afterEach
toregisterCleanupTask
API to clean our tests. However withregisterCleanupTask
, tasks are executed in the wrong order:After
Changes
Testing
I have gone over the contributing documentation.