1166774 - Stop using applyingModification in styleinspector tests

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Alexandre Poirot [:ochameau]]

http://mxr.mozilla.org/mozilla-central/search?string=applyingModification
There is quite a bunch of them! And it is very error prone!
From what I've seen many tests are wrong, and either do not wait or are pretenting to wait whereas there is no actual modification being made!

Instead, like many other tests (and even some of these tests), we should be listening for ruleview-changed.

The issue with this transient attribute is:
- it is private,
- deep, in ruleview.rule
- transient and sometimes null, and that's the main error we have!
  Sometimes we do `yield view.rule.applyingModication`, whereas it is null and so, do not wait. In many cases, we fetch applyingModification promise *before* running the code that will actually create it, so we are saving nothing and then yield on undefined/null, so that we don't wait. I've even seen wrong assertions after that. Asserting for wrong values, because we weren't waiting for the actual state modification :s

I think we can get rid of all test usages!
Also, we should add some new wait, in order to correctly wait for test end,
as it broke my toolbox cleanup patch from bug 1161072.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebd3fa659199

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: patch

rebased.

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8608777 [details] [diff] [review]
patch

You may discover in this patch that there is more ruleview updates than expected,
this isn't the goal of this patch to address that. The only mission here is to have sane tests, without any pending requests!
I picked you randomly as inspector expert and all things related to it,
but feel free to redirect the upcoming reviews...

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8608777 [details] [diff] [review]
patch

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

This all looks pretty good.
Can you just take a look at whether we can introduce a new event instead of waiting for several "changed" events in a row (see comment below).

::: browser/devtools/styleinspector/rule-view.js
@@ +716,5 @@
> +                 .then(() => {
> +                   // Ensure dispatching a ruleview-changed event
> +                   // also for previews
> +                   this.elementStyle._changed()
> +                 });

This seems like the right fix to do, but the formatting is odd, why not:

modifications.apply().then(() => {
  // Make sure we dispatch a ruleview-changed event also for previews.
  this.elementStyle._changed();
});

Also, missing semicolon after _changed()

(yes this is very nit, but I'm working on adding eslint at the moment, so I look at this a lot :) )

::: browser/devtools/styleinspector/test/browser_ruleview_edit-property-commit.js
@@ +66,2 @@
>    is(inplaceEditor(propEditor.valueSpan), editor, "Focused editor should be the value span.");
> +  // Focusing the input dispatch a "starts editing" event

s/dispatch/dispatches

@@ +66,3 @@
>    is(inplaceEditor(propEditor.valueSpan), editor, "Focused editor should be the value span.");
> +  // Focusing the input dispatch a "starts editing" event
> +  // and so update the property value

s/update/updates

::: browser/devtools/styleinspector/test/browser_ruleview_edit-property-increments.js
@@ +153,2 @@
>    let editor = yield focusEditableField(propertyEditor.valueSpan);
> +  yield onRuleViewChanged;

It looks like this should be done inside focusEditableField instead. A lot of other tests need this and may get it wrong too.

::: browser/devtools/styleinspector/test/browser_ruleview_filtereditor-appears-on-swatch-click.js
@@ +22,3 @@
>    swatch.click();
>    yield onShow;
> +  // Clicking on swatch run a preview of the current value

s/run/runs

@@ +22,4 @@
>    swatch.click();
>    yield onShow;
> +  // Clicking on swatch run a preview of the current value
> +  // and update the rule-view

s/update/updates

::: browser/devtools/styleinspector/test/browser_ruleview_multiple-properties-unfinished_01.js
@@ +37,3 @@
>  function* testCreateNewMultiUnfinished(inspector, ruleEditor, view) {
>    let onMutation = inspector.once("markupmutation");
> +  // There is 6 rule-view updates, one for the rule view creation,

Shouldn't we introduce a new event in rule-view that fires at the end of it all? Waiting for 6 events seems random and prone to break in the future.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5e810968376

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #4)
> Comment on attachment 8608777 [details] [diff] [review]
> :::
> browser/devtools/styleinspector/test/browser_ruleview_edit-property-
> increments.js
> @@ +153,2 @@
> >    let editor = yield focusEditableField(propertyEditor.valueSpan);
> > +  yield onRuleViewChanged;
> 
> It looks like this should be done inside focusEditableField instead. A lot
> of other tests need this and may get it wrong too.

Yep... I also did same thing for simulateColorPickerChange!
I end up modifying much more tests in the patch.

> :::
> browser/devtools/styleinspector/test/browser_ruleview_multiple-properties-
> unfinished_01.js
> @@ +37,3 @@
> >  function* testCreateNewMultiUnfinished(inspector, ruleEditor, view) {
> >    let onMutation = inspector.once("markupmutation");
> > +  // There is 6 rule-view updates, one for the rule view creation,
> 
> Shouldn't we introduce a new event in rule-view that fires at the end of it
> all? Waiting for 6 events seems random and prone to break in the future.

I think that's ok, the test is just highlighting the facts.
The rule-view is updates too many times!
I opened bug 1168369 in order to figure this out.

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8610540 [details] [diff] [review]
patch v2

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

The interdiff looks nice, thanks for this. And, as discussed, I'm ok to deal with the 6-times updating problem in the follow-up bug you filed.

::: browser/devtools/styleinspector/test/head.js
@@ +425,5 @@
> +  let onEdit = inplaceEditor(editable.ownerDocument.activeElement);
> +
> +  // Focusing the name or value input is going to fire a preview and update the rule view
> +  if (editable.classList.contains("ruleview-propertyname") ||
> +      editable.classList.contains("ruleview-propertyvalue")) {

nit: Could you create a boolean var for this condition so you can also use it to wrap the onRuleViewChanged initialization. I guess you don't even want to listen once to "ruleview-changed" if that's falsy.

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c304991e2998

Comment 8

[Pulsebot]

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

Comment 9

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/25932df8b78c for frequent e10s failures in browser_ruleview_colorpicker-edit-gradient.js like https://treeherder.mozilla.org/logviewer.html#?job_id=3222807&repo=fx-team

Comment 11

[Alexandre Poirot [:ochameau]]

Can't reproduce locally, nor on try :x
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=c304991e2998
Unless that's related to some very recent change to master as this try is on top of yesterday's tip

Another try with some debug logs, on top of more recent tip, still pending:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=7747bdb84f11

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1166774.patch

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

Since bug 1139058 landed ahead of this, I added a patch that will adjust the tests touched in bug 1139058 to the new style. 

Interdiff: https://bugzilla.mozilla.org/attachment.cgi?oldid=8610570&action=interdiff&newid=8612136&headers=1

Comment 14

[Alexandre Poirot [:ochameau]]

So it looks really green:
  with a debug patch
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=7747bdb84f11
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dedf6079588
  with deps that needs this patch
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b8860d85173
  
And a new one, with gl patch:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=da8233764050

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

Rebased with attachment 8612136 [details] [diff] [review].

Comment 16

[Alexandre Poirot [:ochameau]]

A triggerred many mochitest-devtools and couldn't reproduce this intermittent once,
so I'm trying to reland it.

Comment 17

[Pulsebot]

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

Comment 18

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3294835&repo=fx-team

Comment 19

[Pulsebot]

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

Comment 20

[Alexandre Poirot [:ochameau]]

I've been fooled again by treeherder readability!
But philor helped me by highlighting that's not a try vs fx-team issue...
It's failing on e10s while I wasn't asking for e10s mochitest-devtools to run on try.

Comment 21

[Alexandre Poirot [:ochameau]]

New try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b4619fbcc94
I'm not able to reproduce this failure locally, last time I push to try it was also passing :/

Comment 22

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

Tentative fix...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56287ffdbcea

Comment 23

[Alexandre Poirot [:ochameau]]

Attachment: patch v6

New patch that seems to pass:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=56287ffdbcea

Another try, just to be sure...
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb77c999f083

Comment 24

[Alexandre Poirot [:ochameau]]

Comment on attachment 8631491 [details] [diff] [review]
patch v6

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

Brian, Do you mind stamping this patch?
I highlighter the only one change I made compared to the patch Patrick reviewed.

::: browser/devtools/styleinspector/test/browser_ruleview_colorpicker-edit-gradient.js
@@ +66,5 @@
> +    element: content.document.body,
> +    name: "backgroundImage",
> +    value: "linear-gradient(to left, rgb(1, 1, 1) 25%, rgb(51, 51, 51) 95%, rgb(0, 0, 0) 100%)"
> +  };
> +  yield simulateColorPickerChange(view, cPicker, [1, 1, 1, 1], change);

I just added this `change` callback, to ensure blocking on simulateColorPickerChange until the change is applied to the remote content document.

Comment 25

[Brian Grinstead [:bgrins]]

Comment on attachment 8631491 [details] [diff] [review]
patch v6

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

::: browser/devtools/styleinspector/test/browser_ruleview_colorpicker-edit-gradient.js
@@ +66,5 @@
> +    element: content.document.body,
> +    name: "backgroundImage",
> +    value: "linear-gradient(to left, rgb(1, 1, 1) 25%, rgb(51, 51, 51) 95%, rgb(0, 0, 0) 100%)"
> +  };
> +  yield simulateColorPickerChange(view, cPicker, [1, 1, 1, 1], change);

rs=me assuming this is the only change (interdiff isn't working unfortunately: https://bugzilla.mozilla.org/attachment.cgi?oldid=8630425&action=interdiff&newid=8631491&headers=1)

Comment 26

[Alexandre Poirot [:ochameau]]

Try looks really green, let's try to land this new patch...
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb77c999f083

Comment 27

[Pulsebot]

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

Comment 28

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

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