Fix animation options karma test #7069
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
embarks
added
P1
|
Size Change: +110 B (0%) Total Size: 1.66 MB ℹ️ View Unchanged
|
Looks like we already do the same in In other places we just ignore the errors (as most people seem to do) Also https://stackoverflow.com/questions/49384120/resizeobserver-loop-limit-exceeded#comment118281231_58701523 is worth reading. |
I think if anything is the outcome of last week's bummer of having to disable a bunch of tests it's this - we really should start documenting this better. |
I'll add some documentation! I saw the cypress fix, which is nice, but I'm not sure how to add an event handler for uncaught exceptions for jest/karma... I did some searching and I'm not sure if it runs on |
samwhale
approved these changes
Apr 6, 2021
samwhale
reviewed
Apr 6, 2021
Comment on lines
-78
to
+79
| // TODO #6953 | ||
| // eslint-disable-next-line jasmine/no-disabled-tests | ||
| xit('plays the animation when a control in the panel is changed.', async function () { | ||
| it('plays the animation when a control in the panel is changed.', async function () { |
There was a problem hiding this comment.
There's another test right above this that I think you can unskip too!
embarks
force-pushed
the
fix/6953-animations
branch
from
abcb461 to
8baee02
Compare
BrittanyIRL
approved these changes
Apr 6, 2021
embarks
force-pushed
the
fix/6953-animations
branch
from
68fc6c0 to
1b853d8
Compare
added 6 commits
April 7, 2021 11:44
… console errors when none selected
…to handle nulls (introduced from new timers)
embarks
force-pushed
the
fix/6953-animations
branch
from
a98e8fa to
57eefac
Compare
Codecov Report
@@ Coverage Diff @@
## main #7069 +/- ##
===========================================
+ Coverage 65.96% 81.90% +15.93%
===========================================
Files 1129 1133 +4
Lines 17523 17554 +31
===========================================
+ Hits 11559 14377 +2818
+ Misses 5964 3177 -2787
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
This PR re-enables a flaky test and introduces some fixes to help the test pass. The test was for checking that the animation plays after the effect chooser is changed. The other fixes are detailed in Relevant Technical Choices.
Summary
The error which was causing the test to fail reads as follows:
ResizeObserver loop limit exceeded thrownThis was due to a "benign error" with the ResizeObserver, some discussion can be found here in a github issue thread for the resize-observer and here-- in the stack overflow thread which details the chosen solution.
The error is intermittent. I ran the karma test suite 10 times on
mainand saw a failure once. In order to test I found where the resize observer was being instantiated when interacting with the effect chooser dropdown and running and added therequestAnimationFramethere which prevents the error from occurring.In order to verify, I re-ran the same test suite 30 times and never saw the error locally.
Relevant Technical Choices
I noticed a couple other problems while I was replicating:
To-do
User-facing changes
This is a bug fix.
Testing Instructions
QA
UAT
Reviews
Does this PR have a security-related impact?
Does this PR change what data or activity we track or use?
Does this PR have a legal-related impact?
Checklist
Type: XYZlabel to the PRFixes #6953