1245420 - devtools/client/framework/test aren't properly sorted

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Comment 1

[Patrick Brosset <:pbro>]

And browser_toolbox_toggle.js is too hard to read in its current form. This test needs rewriting.

Comment 2

[Patrick Brosset <:pbro>]

Re-ordering tests made some of them fail because they would expect a given default panel to be selected.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1245420 - Sort devtools/client/framework tests and clean-up browser_toolbox_toggle.js; r=ochameau

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

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8715221 [details]
MozReview Request: Bug 1245420 - Sort devtools/client/framework tests and clean-up browser_toolbox_toggle.js; r=ochameau

https://reviewboard.mozilla.org/r/33369/#review30085

Whaaaa this test was so hard to follow, this is so much simplier!!
nit: The commit message isn't perfect, you cleaned up more browser_toolbox_tabsswitch_shortcuts.js than the others.

::: devtools/client/framework/test/browser_toolbox_toggle.js:14
(Diff revision 1)
> +  Services.prefs.clearUserPref("devtools.toolbox.selectedTool");

I don't get this one?
It looks like a previous test mess up with this pref?
I imagine that will be hard to ensure all test to reset this pref. Ideally they would. It wouldn't be that hard if we have a shared head.js, but I think we are not there yet?
Otherwise here I see two options:
* Add a comment about this early cleaUserPref (this is not a common practice and a workaround)
* Use SpecialPowers.pushPrefEnv with an explicit selectedTool value.

Comment 5

[Patrick Brosset <:pbro>]

Pending try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f268685dd543&group_state=expanded

(In reply to Alexandre Poirot [:ochameau] from comment #4)
> ::: devtools/client/framework/test/browser_toolbox_toggle.js:14
> (Diff revision 1)
> > +  Services.prefs.clearUserPref("devtools.toolbox.selectedTool");
> 
> I don't get this one?
> It looks like a previous test mess up with this pref?
> I imagine that will be hard to ensure all test to reset this pref. Ideally
> they would. It wouldn't be that hard if we have a shared head.js, but I
> think we are not there yet?
> Otherwise here I see two options:
> * Add a comment about this early cleaUserPref (this is not a common practice
> and a workaround)
> * Use SpecialPowers.pushPrefEnv with an explicit selectedTool value.
Good point. And we do have a shared head now, so I can easily clear this pref there, when a test ends. I hope that's not going to break dozens of other tests that implicitly assumed a given default tool would be selected.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3cd839e515b9

Comment 7

[Patrick Brosset <:pbro>]

Ended up adding a comment in the test instead, and filing bug 1245784 because I was seeing some weird crashes on TRY. I'm not sure they were related, but I prefer not to risk it.
I'll test again in bug 1245784.

Comment 8

[Carsten Book [:Tomcat]]

had to back this out in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=073aa15f8ea4 - seems one of this 2 pushes caused test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7041768&repo=fx-team

Comment 9

[Patrick Brosset <:pbro>]

I have a fix for this issue and I've pushed it to try with many retriggers of the corresponding dt job:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a528779432bd&group_state=expanded&selectedJob=16382638
The test failure is gone now, but I'm now seeing fairly frequent failures of browser_markup_load_01.js on linux debug builds (for which there are no bugs on file yet).
Knowing that my patch only changes things in this directory devtools/client/framework/test/ there's absolutely no reasons it could cause this other test to fail now.

Comment 10

[Pulsebot]

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

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a433d06618ad