Closed Bug 1448732 Opened 4 years ago Closed 4 years ago

Reduce the testing time for new animation inspector

Categories

(DevTools :: Inspector: Animations, enhancement)

enhancement
Not set
normal

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

Details

Attachments

(4 files)

Sometimes, the tests got timeout error in especially windows10-64-ccov, Linux x64 JSDCov in try. We should try to reduce the taking time for testing.
I have tried that remove unnecessary animated nodes once before opening the animation inspector in browser_animation_keyframes-progress-bar.js. As the result, in this test case, it seems we can reduce approximately 30% like below. I'll apply same way to all tests as first trying.

Linux x64 debug
* New AVG: 53533ms
  (50629ms[1], 58904ms[2], 52563ms[3], 52656ms[4], 52914ms[5])
* Old AVG: 75223ms
  (76708ms[6], 74488ms[7], 73521ms[8], 76622ms[9], 74776ms[10])

Windows 10 x64 debug
* New AVG: 25933ms
  (26183ms[11], 27547ms[12], 25695ms[13], 24573ms[14], 25669ms[15])
* Old AVG: 35757ms
  (30535ms[16], 36563ms[17], 38414ms[18], 37340ms[19], 35933ms[20])

[1]  https://treeherder.mozilla.org/logviewer.html#?job_id=170221631&repo=try&lineNumber=5209
[2]  https://treeherder.mozilla.org/logviewer.html#?job_id=170241096&repo=try&lineNumber=5247
[3]  https://treeherder.mozilla.org/logviewer.html#?job_id=170241098&repo=try&lineNumber=5234
[4]  https://treeherder.mozilla.org/logviewer.html#?job_id=170241099&repo=try&lineNumber=5219
[5]  https://treeherder.mozilla.org/logviewer.html#?job_id=170241100&repo=try&lineNumber=5226

[6]  https://treeherder.mozilla.org/logviewer.html#?job_id=170215528&repo=try&lineNumber=5290
[7]  https://treeherder.mozilla.org/logviewer.html#?job_id=170241126&repo=try&lineNumber=5264
[8]  https://treeherder.mozilla.org/logviewer.html#?job_id=170241128&repo=try&lineNumber=5247
[9]  https://treeherder.mozilla.org/logviewer.html#?job_id=170241129&repo=try&lineNumber=5271 
[10] https://treeherder.mozilla.org/logviewer.html#?job_id=170241448&repo=try&lineNumber=5289

[11] https://treeherder.mozilla.org/logviewer.html#?job_id=170220834&repo=try&lineNumber=8936
[12] https://treeherder.mozilla.org/logviewer.html#?job_id=170241443&repo=try&lineNumber=8385
[13] https://treeherder.mozilla.org/logviewer.html#?job_id=170241444&repo=try&lineNumber=9019
[14] https://treeherder.mozilla.org/logviewer.html#?job_id=170241445&repo=try&lineNumber=8862
[15] https://treeherder.mozilla.org/logviewer.html#?job_id=170241446&repo=try&lineNumber=8855

[16] https://treeherder.mozilla.org/logviewer.html#?job_id=170215596&repo=try&lineNumber=8889
[17] https://treeherder.mozilla.org/logviewer.html#?job_id=170241347&repo=try&lineNumber=9491
[18] https://treeherder.mozilla.org/logviewer.html#?job_id=170241348&repo=try&lineNumber=9423
[19] https://treeherder.mozilla.org/logviewer.html#?job_id=170241349&repo=try&lineNumber=9360
[20] https://treeherder.mozilla.org/logviewer.html#?job_id=170241350&repo=try&lineNumber=9478
Also, I have tried one more thing the way for selecting an animation. That clicks on summary graph instead of target element. It's reduce the rendering for summary graph. As result, we could reduce the testing time overall. I'd like to request the review.

I paste the average time each tests that modified as the result of windows10-64-ccov debug on the try. (New[1] Old[2])

test name (no browser_animation_ prefix)                        NEW(ms) OLD      diff   % (NEW / OLD * 100)
animated-property-list.js                                	27433	50016	-22583	45.1 faster
animated-property-list_unchanged-items.js                 	22590	44103	-21513	48.8
animated-property-name.js                                	18473	34436	-15963	46.4
animation-detail_close-button.js                         	17121	53915	-36794	68.2
animation-detail_title.js                                	22553	73911	-51358	69.5
animation-detail_visibility.js                           	24053	94207	-70154	74.5
animation-list.js                                          	20095	33725	-13630	40.4
animation-target.js                                       	18426	32589	-14163	43.5
animation-timeline-tick.js                               	16254	26482	-10228	38.6
current-time-label.js                                    	18921	71200	-52279	73.4
current-time-scrubber.js                                 	30273	53189	-22916	43.1
empty_on_invalid_nodes.js                                     	20266	32049	-11783	36.8
keyframes-graph_computed-value-path.js                      	69850	94928	-25078	26.4
keyframes-graph_computed-value-path_easing-hint.js       	44377	75478	-31101	41.2
keyframes-graph_keyframe-marker.js                          	30083	52566	-22483	42.8
keyframes-progress-bar.js                                 	94616	170937	-76321	44.6
logic_auto-stop.js                                       	37726	61798	-24072	39.0
logic_mutations.js                                       	36873	75831	-38958	51.4
summary-graph_animation-name.js                          	15487	53158	-37671	70.9
summary-graph_compositor.js                              	30862	62746	-31884	50.9
summary-graph_computed-timing-path.js	                        50406	60501	-10095	16.7
summary-graph_computed-timing-path_different-timescale.js	34634	52880	-18246	34.5
summary-graph_delay-sign.js	                                24556	65152	-40596	62.3
summary-graph_effect-timing-path.js	                        19909	62759	-42850	68.3
summary-graph_end-delay-sign.js	                                22314	64018	-41704	65.1
summary-graph_negative-delay-path.js	                        22833	63340	-40507	64.0
summary-graph_negative-end-delay-path.js	                        20402	62931	-42529	67.6
summary-graph_tooltip.js                                 	36990	63007	-26017	41.3

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=877f7d7a8db352b1b5884bdd58aadc2487091dcb
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=831ea912cd2437ca37b85d8f089ff1ea025c5715
Comment on attachment 8963441 [details]
Bug 1448732 - Part 0: Modify variables and code format for consistency.

https://reviewboard.mozilla.org/r/232352/#review237904

::: commit-message-21682:5
(Diff revision 1)
> +Bug 1448732 - Part 0: Modify variables and code format for consistency. r?pbro
> +
> +In this patch, modify the following things:
> +
> +* Modify bariable name of test data list to "TEST_DATA"

s/bariable/variable
Attachment #8963441 - Flags: review?(pbrosset) → review+
Comment on attachment 8963442 [details]
Bug 1448732 - Part 1: Remove extra animated elements before open animation inspector.

https://reviewboard.mozilla.org/r/232354/#review237906

I like the approach of making each test much more standalone, and only having some animations running instead of all. I'm a bit unsure about *removing* animations though. It feels a bit sad to have to do that.
It would be better for each test to have its own test document instead. But creating all of the new HTML files might be a bit time consuming, maybe too much for this bug. So I'll leave that up to you.

::: devtools/client/inspector/animation/test/doc_frame_script.js:23
(Diff revision 1)
> +  sendAsyncMessage("Test:RemoveAnimatedElementsExcept");
> +});
> +
> +function isRemovableElement(animation, classes) {
> +  for (const clazz of classes) {
> +    if (animation.effect.target.classList.contains(clazz)) {

See my comment about using a selector instead of a class. 
If you do this, you can then use the `DOMNode.matches` function here instead.

::: devtools/client/inspector/animation/test/head.js:89
(Diff revision 1)
> +const removeAnimatedElementsExcept = async function(classes) {
> +  return executeInContent("Test:RemoveAnimatedElementsExcept", { classes });
> +};

Making this work for classes only might be a bit of a problem in the future. Why not make this a list of `selectors` instead?
Just rename classes to selectors and let consumers pass any valid CSS selector there.
Attachment #8963442 - Flags: review?(pbrosset) → review+
Comment on attachment 8963443 [details]
Bug 1448732 - Part 2: Open detail pane by clicking on summary graph instead of selecting the node.

https://reviewboard.mozilla.org/r/232356/#review237914

::: devtools/client/inspector/animation/test/head.js:109
(Diff revision 1)
> -  const x = bounds.width / 2;
> -  const y = bounds.height / 2;
> -  EventUtils.synthesizeMouse(summaryGraphEl, x, y, {}, summaryGraphEl.ownerGlobal);
>  
> -  await waitForAnimationDetail(animationInspector);
> +/**
> + * Click on an animation by class name of node which is target element of animation.

Same comment as in a previous review: it feels a bit constraining to have to only use classnames. What if we add a test with an element that has an ID.
I would refactor this whole part (and the other functions) to allow selectors instead.
This way it would also work with "#test-id" for example.
Attachment #8963443 - Flags: review?(pbrosset) → review+
Comment on attachment 8963444 [details]
Bug 1448732 - Part 3: Adjust requestLongerTimeout.

https://reviewboard.mozilla.org/r/232358/#review237924
Attachment #8963444 - Flags: review?(pbrosset) → review+
Comment on attachment 8963442 [details]
Bug 1448732 - Part 1: Remove extra animated elements before open animation inspector.

https://reviewboard.mozilla.org/r/232354/#review237906

Thanks, Patrick.
I wanted to re-use the document as much as possible.
But actually, the removing as well takes extra time.
As you said, we might be better to prepare the document for each.
Please let me do in another bug.

And I will address other points.
Thanks!
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ef7e2cd46f5
Part 0: Modify variables and code format for consistency. r=pbro
https://hg.mozilla.org/integration/autoland/rev/64c98c77e0f9
Part 1: Remove extra animated elements before open animation inspector. r=pbro
https://hg.mozilla.org/integration/autoland/rev/d51d434ff1a9
Part 2: Open detail pane by clicking on summary graph instead of selecting the node. r=pbro
https://hg.mozilla.org/integration/autoland/rev/59615d0d8365
Part 3: Adjust requestLongerTimeout. r=pbro
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.