914861 - browser_toolbox_options.js times out when a tool in defaultTools is disabled by default

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Jake Bailey]

browser_toolbox_options assumes that all tools are enabled by default. The toggleTools function adds a listener for the "tool-unregistered" event, but the synthesized click that follows triggers a "tool-registered" event for tools which are disabled by default. The test times out waiting for the "tool-unregistered" event, and the output doesn't do much to indicate what the actual problem is.

The test should either explicitly test that all default tools are enabled at the beginning of the test, or it should not depend on the default enablement state of the default tools.

Comment 1

[Jake Bailey]

Attachment: Patch

Makes the test not dependent on the initial state of the tool prefs.

Comment 2

[Jake Bailey]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=ec1265343d7a

Comment 3

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 804736 [details] [diff] [review]
Patch

Review of attachment 804736 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/framework/test/browser_toolbox_options.js
@@ +1,4 @@
>  /* Any copyright is dedicated to the Public Domain.
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +let doc = null, toolbox = null, panelWin = null, index = 0, prefValues = [], prefNodes = [], toggleInReverse = false;

could you break these up a bit please? That line's gotten ridiculous.

@@ +89,1 @@
>    prefNodes = prefNodes.sort(() => Math.random() > 0.5 ? 1: -1);

no.

@@ +117,5 @@
> +    toggleInReverse ? index-- : index++;
> +    if (index === prefNodes.length) {
> +      toggleInReverse = true;
> +      index--;
> +    }

I really don't like what this test is doing. It's impossible to follow having state in this global index variable.

I'm bailing out of the review to reread why you're doing this.

Comment 4

[Rob Campbell [:rc] (:robcee)]

yeah, I don't like adding randomness to our unittests. I think I'd like something deterministic here.

Comment 5

[Jake Bailey]

The randomness was already there (bug 915939). I just reworded the comment.

Benvie, anton, and I talked with Optimizer about it on IRC. The point of randomizing the order of toggling the tools is to ensure that when tools are removed or added, their toolbox tab is removed or re-added in sorted order.

So, it's testing this code: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#535

The randomness isn't a good solution--I'm writing a version of this test that just tests edge cases instead.

Comment 6

[Jake Bailey]

Attachment: Patch

Since this test and I are such good friends now, I tried to resolve both issues and make the test easier to understand. I removed most of the global state and used promises to (hopefully) make the test easier to follow.

Instead of disabling and re-enabling all the tools in a randomized order, the test now tries to test edge cases (adding a toolbox tab when no tabs are present; adding a tab to the beginning, middle, and end of the tab list). This should catch most failures that the randomized test could have caught.

Comment 7

[Jake Bailey]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=2713c1c5c66e

Comment 8

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 810099 [details] [diff] [review]
Patch

Review of attachment 810099 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for doing this. I like this test much better now. :)

r+ with the below changes.

::: browser/devtools/framework/test/browser_toolbox_options.js
@@ +15,5 @@
> +      .then(testSelectTool)
> +      .then(testOptionsShortcut)
> +      .then(testOptions)
> +      .then(testToggleTools)
> +      .then(cleanup);

you should probably add a .then(null, errorHandler) to the end of this chain to capture any badness that might occur.

e.g., see, http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/test/browser_dbg_listtabs-01.js#32

@@ +113,5 @@
> +    p = p.then(toggleTool.bind(null, node));
> +  }
> +  for (let node of enabledTools) {
> +    p = p.then(toggleTool.bind(null, node));
> +  }

so good we needed it twice?

I guess we're toggling so the first pass disables(?) then second enables?

still a little weird. Maybe update the comment to explain what's happening a little more clearly so people don't think it's a paste error.

@@ +126,5 @@
> +       .then(toggleTool.bind(null, firstTool))
> +       .then(toggleTool.bind(null, middleTool))
> +       .then(toggleTool.bind(null, middleTool))
> +       .then(toggleTool.bind(null, lastTool))
> +       .then(toggleTool.bind(null, lastTool));

again, add another .then(null, errorHandler) in case Something Bad Happened™.

Comment 9

[Jake Bailey]

(In reply to Rob Campbell [:rc] (:robcee) from comment #8)
> @@ +126,5 @@
> > +       .then(toggleTool.bind(null, firstTool))
> > +       .then(toggleTool.bind(null, middleTool))
> > +       .then(toggleTool.bind(null, middleTool))
> > +       .then(toggleTool.bind(null, lastTool))
> > +       .then(toggleTool.bind(null, lastTool));
> 
> again, add another .then(null, errorHandler) in case Something Bad Happened™.

testToggleTools adds all these .then()s into the main promise chain from onLoad, so just adding the errorHandler to the end of the onLoad chain is enough to catch errors thrown by toggleTool.

Comment 10

[Jake Bailey]

Attachment: Patch

Adds requested comments and error handler. I also reset all modified preferences in the cleanup function, since this test can cause a lot of failures in following tests if it leaves a toolbox tab enabled/disabled when it shouldn't be.

Comment 11

[Jake Bailey]

Attachment: Patch

Rebased onto fx-team.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/96e6d3d323cd

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/96e6d3d323cd

Comment 14

[Rob Campbell [:rc] (:robcee)]

Thanks Jake!