1181839 - Migrate browser/devtools/animationinspector tests to shared-head.js

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Brian Grinstead [:bgrins]]

We should import shared-head.js in animationinspector/test/head.js and remove any newly unnecessary code after that.

Comment 1

[Patrick Brosset <:pbro>]

I've done for the rule-view, computed-view, fonts inspector, layout-view and markup-view. So I know how to do this pretty easily. I might take a look at that bug soon(ish) if I've got time. If not, anyone's free to pick that up and I'll mentor them.
Know however that this isn't your usual issue fixing bug or new feature bug, this is a test refactoring bug.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Patrick, did you start to work on this or can I take it ?

Comment 3

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe from comment #2)
> Patrick, did you start to work on this or can I take it ?
No I haven't. Feel free to take this on.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

I see that 2 functions in the animationinspector head.js are also created in other head.js files, waitForContentMessage and executeInContent.

https://dxr.mozilla.org/mozilla-central/search?q=function+waitForContentMessage&redirect=false&case=false
https://dxr.mozilla.org/mozilla-central/search?q=function+executeInContent&redirect=false&case=false

From what I can see, they all look pretty similar, and I wonder if I could move them to shared-head.js .
If so, I think I should do 2 commits ( which would then create 2 patches for MozReview I think) :
- move the functions, remove declarations in head.js files that import share-head and commit
- import shared-head.js in animationinspector's head.js and remove code duplication ( thus, remove the 2 functions ).

Am I right ? Or is there a good reason for those functions not to be in shared-head.js yet ?

Comment 5

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe from comment #4)
> I see that 2 functions in the animationinspector head.js are also created in
> other head.js files, waitForContentMessage and executeInContent.
> 
> https://dxr.mozilla.org/mozilla-central/
> search?q=function+waitForContentMessage&redirect=false&case=false
> https://dxr.mozilla.org/mozilla-central/
> search?q=function+executeInContent&redirect=false&case=false
> 
> From what I can see, they all look pretty similar, and I wonder if I could
> move them to shared-head.js .
> If so, I think I should do 2 commits ( which would then create 2 patches for
> MozReview I think) :
> - move the functions, remove declarations in head.js files that import
> share-head and commit
> - import shared-head.js in animationinspector's head.js and remove code
> duplication ( thus, remove the 2 functions ).
> 
> Am I right ? Or is there a good reason for those functions not to be in
> shared-head.js yet ?
The longer term solution here is to do what I did for ruleview/computedview/fontinspector/markupview tests:
- import head.js from inspector/test/head.js
- which, itself, imports the shared-head.js
This is useful because the animation-inspector, being a sidebar that lives in the inspector, requires a lot of the same things than the inspector itself (like, open the inspector, selecting a node, etc...).
And then:
- using the test-actor to replace occurrences of executeInContent
executeInContent is sort of obsolete now. What it does is it loads a frame-script in the test content process and allows you to send messages to it to do various things in a way that works with e10s. But, there's a couple of better ways now:
- using ContentTask.spawn instead
- or using the test-actor
I prefer using the test-actor because we started introducing it in inspector-related tests last year. The end goal of it being to be able to run tests on remote devices directly.

So, coming back to importing the inspector's head.js file, you can do this with:

/* import-globals-from ../../inspector/test/head.js */
// Import the inspector's head.js first (which itself imports shared-head.js).
Services.scriptloader.loadSubScript(
  "chrome://mochitests/content/browser/devtools/client/inspector/test/head.js",
  this);

Note the comment on the first line: this is to tell eslint where to look for global symbols.

Once done, you'll be able to do a lot of cleanup in the animationinspector's head.js file. Here are few things that come to mind:
- remove imports like: Cu, require, gDevTools, promise, TargetFactory, console, DevToolsUtils
- remove waitForExplicitFinish
- remove TEST_URL_ROOT and replace all occurrences of it in animationinspector tests with URL_ROOT which is already defined in shared-head.js
- remove the commented out devtools.dump.emit pref thing
- remove the DevToolsUtils.testing related things
- remove addTab
- remove various helpers already defined in inspector's head.js: selectNode, getNodeFront
- openAnimationInspector should be simplified greatly by calling openInspectorSidebarTab defined in inspector's head.js (you can probably look at inspector/rules/test/head.js for a similar thing). Which also means that things like waitForToolboxFrameFocus and hasSideBarTab should be removable
- remove once

For now, let's keep executeInContent, don't worry about it, replacing this with the test-actor requires probably a lot more work to many tests. We can do this in a second step.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: MozReview Request: Bug 1181839 - Includes inspector/test/head.js ( which imports shared-head.js )

in animationinspector's tests and removes code duplication. r?pbro

Review commit: https://reviewboard.mozilla.org/r/38953/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38953/

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Some comments about the cleaning : 
- I couldn't get rid of selectNode, as we want to wait for the animations to be displayed before moving on. I renamed it safeSelectNode and call selectNode inside. I could not found a better name for it, but there should be one.
- I did not remove addTab for the same reasons.

I ran ./mach test /Users/nicolaschevobbe/Projects/fx-team/devtools/client/animationinspector/test/* and all the test pass.

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8728501 [details]
MozReview Request: Bug 1181839 - Includes inspector/test/head.js ( which imports shared-head.js )

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38953/diff/1-2/

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

https://reviewboard.mozilla.org/r/38953/#review35785

::: devtools/client/animationinspector/test/browser_animation_panel_exists.js:10
(Diff revisions 1 - 2)
> -  yield addAnimationTab("data:text/html;charset=utf-8,welcome to the animation panel");
> +  yield addAnimationTab(
> +    "data:text/html;charset=utf-8,welcome to the animation panel");

fixes eslint max-length error

Comment 10

[Patrick Brosset <:pbro>]

https://reviewboard.mozilla.org/r/38953/#review35791

This looks pretty good, thanks for cleaning this up.
I've made a couple of comments below:
- let's try and keep `addTab` named `addTab` (by doing what we did in the ruleview tests)
- and let's rename `safeSelectNode` to something a bit more self-explanatory.
Shouldn't be too long to fix with a mass search/replace on the whole directory :)
Apart from this, it's fine, happy to R+ at the next review and get this landed.

::: devtools/client/animationinspector/test/head.js:28
(Diff revision 2)
>  // Uncomment this pref to dump all devtools emitted events to the console.
>  // Services.prefs.setBoolPref("devtools.dump.emit", true);

This also appears in shared-head.js, so maybe you can remove it from here.

::: devtools/client/animationinspector/test/head.js:42
(Diff revision 2)
> -function addTab(url) {
> +var addAnimationTab = Task.async(function*(url) {
>    info("Adding a new tab with URL: '" + url + "'");
> -  let def = promise.defer();
>  
>    window.focus();
>  
>    let tab = window.gBrowser.selectedTab = window.gBrowser.addTab(url);
>    let browser = tab.linkedBrowser;
>  
>    info("Loading the helper frame script " + FRAME_SCRIPT_URL);
>    browser.messageManager.loadFrameScript(FRAME_SCRIPT_URL, false);
>  
>    info("Loading the helper frame script " + COMMON_FRAME_SCRIPT_URL);
>    browser.messageManager.loadFrameScript(COMMON_FRAME_SCRIPT_URL, false);
>  
> -  browser.addEventListener("load", function onload() {
> +  yield once(browser, "load", true);
> -    browser.removeEventListener("load", onload, true);
> -    info("URL '" + url + "' loading complete");
> +  info("URL '" + url + "' loading complete");
> +});

Hmm, I'd prefer to do what devtools\client\inspector\rules\test\head.js does here:
Override the `addTab` function defined in shared-head.js locally, so you can still call it but execute code after it's done.

::: devtools/client/animationinspector/test/head.js:86
(Diff revision 2)
> -var selectNode = Task.async(function*(data, inspector, reason = "test") {
> +var safeSelectNode = Task.async(function*(data, inspector, reason = "test") {

I have 2 proposals for this:

- Either name this `selectNodeAndWaitForAnimations` so it's really self explanatory.
- Or, override the inspector's `selectNode` this way:

```
var inspectorSelectNode = selectNode;
selectNode = Task.async(function*(...args) {
  yield inspectorSelectNode(...args);
  // 99% of the times, selectNode is called to select an animated node, and we
  // want to make sure the rest of the test waits for the animations to be
  // properly displayed (wait for all target DOM nodes to be previewed).
  // Even if there are no animations, this is safe to do.
  let {AnimationsPanel} = inspector.sidebar.getWindowForTab(TAB_NAME);
  yield waitForAllAnimationTargets(AnimationsPanel);
});
```

This way, no need to change any of the tests, they can still call `selectNode`, so it's transparent.
But also, maybe more confusing for people?
I like the `selectNodeAndWaitForAnimations` option, because you really know what you're doing when writing a test, and you still have the option to just call `selectNode` in cases where you'd need to write a test that doesn't wait for animations on purpose.

::: devtools/client/animationinspector/test/head.js
(Diff revision 2)
> -

Why removing the empty line here? This seems unrelated, and I kind of like it this way, makes it a bit easier to read.

::: devtools/client/animationinspector/test/head.js
(Diff revision 2)
> -

Same here.

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

https://reviewboard.mozilla.org/r/38953/#review35791

> Hmm, I'd prefer to do what devtools\client\inspector\rules\test\head.js does here:
> Override the `addTab` function defined in shared-head.js locally, so you can still call it but execute code after it's done.

Okay.
I refrained to use shared-head.js addTab because of 

```
var addTab = Task.async(function*(url) {
  info("Adding a new tab with URL: " + url);

  let tab = gBrowser.selectedTab = gBrowser.addTab(url);
  yield once(gBrowser.selectedBrowser, "load", true);

  info("Tab added and finished loading");

  return tab;
});
```

We wait for the "load" event to move on, and as we want to load 2 scripts in animationinspector head FRAME_SCRIPT_URL , I was afraid of a having racing conditions if we add them after the "load" event is fired. But I'm maybe wrong about how "load" event works ?

> Why removing the empty line here? This seems unrelated, and I kind of like it this way, makes it a bit easier to read.

Moved too much things around, sorry about that

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe from comment #11)
> We wait for the "load" event to move on, and as we want to load 2 scripts in
> animationinspector head FRAME_SCRIPT_URL , I was afraid of a having racing
> conditions if we add them after the "load" event is fired. But I'm maybe
> wrong about how "load" event works ?
This shouldn't be a problem. You can load frame scripts through the message manager at any point, and messages sent after this will automatically be queued until the script is loaded. So this should be totally transparent.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8728501 [details]
MozReview Request: Bug 1181839 - Includes inspector/test/head.js ( which imports shared-head.js )

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38953/diff/2-3/

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

https://reviewboard.mozilla.org/r/38953/#review35875

I pulled and rebased, so you should be able to apply the patch right away.
I checked that all the test pass ( `./mach test fx-team/devtools/client/animationinspector/test/`), and that there are no eslint errors ( `./mach eslint fx-team/devtools/client/animationinspector/test/*.js` )

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8728501 [details]
MozReview Request: Bug 1181839 - Includes inspector/test/head.js ( which imports shared-head.js )

https://reviewboard.mozilla.org/r/38953/#review35971

This is niiiice! Thanks

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8728501 [details]
MozReview Request: Bug 1181839 - Includes inspector/test/head.js ( which imports shared-head.js )

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38953/diff/3-4/

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11b057f8101e

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11b057f8101e