Closed Bug 1396182 Opened 2 years ago Closed 2 years ago

Intermittent devtools/client/styleeditor/test/browser_styleeditor_opentab.js | The menu item is disabled - Got false, expected true

Categories

(DevTools :: Style Editor, defect, P2)

defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: pbro)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell fixed:other])

Attachments

(1 file)

This started 9 days ago. I have a hunch this started with bug 1379099, because in that bug we added an animation in the sidebar, where the context menu is supposed to be displayed. So maybe this introduced a timing issue when the test tries to show the menu.
Priority: P5 → P2
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Comment on attachment 8906517 [details]
Bug 1396182 - Wait for the styleditor animations to be done before running tests;

https://reviewboard.mozilla.org/r/178268/#review183848

::: devtools/client/styleeditor/test/head.js:91
(Diff revision 2)
> +  // animation is done. So we listen to it here so tests don't have to and can't miss it.
> +  let onMediaListChanged = ui.once("media-list-changed");
> +
> +  // The stylesheet list appears with an animation. Let this animation finish.
> +  let animations = ui._root.getAnimations({subtree: true});
> +  yield Promise.all(animations.map(a => a.finished));

Isn't that related to this async code:
http://searchfox.org/mozilla-central/source/devtools/client/styleeditor/StyleSheetEditor.jsm#129-130     
  this.cssSheet.getMediaRules().then(this._onMediaRulesChanged, console.error);

Then, shouldn't we instead wait for it to finish by introducing StyleSheetEditor.init method returning a promise and we would wait for it to finish from here:
http://searchfox.org/mozilla-central/source/devtools/client/styleeditor/StyleEditorUI.jsm#392
Like we do for fetchSource().

This would allow tracking this on DAMP, and also benefit to any code relying on showToolbox("styleditor").
Attachment #8906517 - Flags: review?(poirot.alex) → review+
r+ as it looks like a reasonable fix just for test, but I would really like us fix showToolbox/DAMP and really reflect what users see. These animations are definitely part of the load time and should be tracked for perf regression.
Comment on attachment 8906517 [details]
Bug 1396182 - Wait for the styleditor animations to be done before running tests;

https://reviewboard.mozilla.org/r/178268/#review183848

> Isn't that related to this async code:
> http://searchfox.org/mozilla-central/source/devtools/client/styleeditor/StyleSheetEditor.jsm#129-130     
>   this.cssSheet.getMediaRules().then(this._onMediaRulesChanged, console.error);
> 
> Then, shouldn't we instead wait for it to finish by introducing StyleSheetEditor.init method returning a promise and we would wait for it to finish from here:
> http://searchfox.org/mozilla-central/source/devtools/client/styleeditor/StyleEditorUI.jsm#392
> Like we do for fetchSource().
> 
> This would allow tracking this on DAMP, and also benefit to any code relying on showToolbox("styleditor").

True. Let's land this test fix as the intermittent is quite frequent. I'll file the next bug to take care of showToolbox being more realistic.
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2b4360780665
Wait for the styleditor animations to be done before running tests; r=ochameau
Blocks: 1399779
Whiteboard: [stockwell fixed:other]
https://hg.mozilla.org/mozilla-central/rev/2b4360780665
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.