1241527 - Fix remaining unhandled rejected promises in rule-view mochitests

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, defect)

Description

[Patrick Brosset <:pbro>]

In bug 1240804, we're trying to re-enable test failures on unhandled rejected promises. It turns out a bunch of rule-view tests have pending requests on test shutdown, so we should fix those.

I'm going to use this bug to attach many patches to fix these problems

I've fixed some tests already in previous bugs. With those fixed, I think the remaining list is:

devtools/client/inspector/rules/test/browser_rules_edit-property-increments.js
devtools/client/inspector/rules/test/browser_rules_edit-property_03.js
devtools/client/inspector/rules/test/browser_rules_edit-property_08.js
devtools/client/inspector/rules/test/browser_rules_editable-field-focus_01.js
devtools/client/inspector/rules/test/browser_rules_editable-field-focus_02.js
devtools/client/inspector/rules/test/browser_rules_filtereditor-commit-on-ENTER.js
devtools/client/inspector/rules/test/browser_rules_multiple-properties-duplicates.js
devtools/client/inspector/rules/test/browser_rules_multiple-properties-priority.js
devtools/client/inspector/rules/test/browser_rules_multiple-properties-unfinished_02.js
devtools/client/inspector/rules/test/browser_rules_multiple_properties_01.js
devtools/client/inspector/rules/test/browser_rules_search-filter_07.js
devtools/client/inspector/rules/test/browser_rules_search-filter_08.js
devtools/client/inspector/rules/test/browser_rules_search-filter_09.js

Comment 1

[Patrick Brosset <:pbro>]

Many of them are due to the test not waiting for long enough before ending. In particular, the "ruleview-changed" event proves useful in a lot of cases.

In order to investigate this issue, it's useful to add this at the end of a test:

Services.prefs.setBoolPref("devtools.dump.emit", true);
console.log("============== START")
yield new Promise(r => setTimeout(r, 5000))
console.log("============== STOP")

This way, the pending requests have time to settle, and with the emit pref ON, you can see all the events that are being emitted during the shutdown phase.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: Bug_1241527_-_1_-_Fix_some_unhandled_rejected_prom.diff

Here's the first patch, fixes some of them. More to come.
This one should be pretty straightforward.
Most fixes are in tests themselves but one: the cssfilter widget used to emit way too much events. When it gets initialized, the filter value gets applied and that causes it to emit one "updated" event per filter function + one at the end. I made it so it would only emit one event only, at the end.

Comment 3

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

Comment on attachment 8710509 [details] [diff] [review]
Bug_1241527_-_1_-_Fix_some_unhandled_rejected_prom.diff

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

::: devtools/client/inspector/rules/test/head.js
@@ +288,5 @@
> + * ruleview-changed event.
> + * @param {Tooltip} tooltip
> + * @param {CSSRuleView} view
> + */
> +function* hideTooltipAndWaitForRuleviewChanged(tooltip, view) {

To stick to our naming conventions, we should capitalize the 'v' and rename the function to hideTooltipAndWaitForRuleViewChanged

Comment 4

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=698c7c6ffe33&group_state=expanded

Comment 5

[Pulsebot]

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

Comment 6

[Patrick Brosset <:pbro>]

Attachment: Bug_1241527_-_2_-_Use_ruleview-changed_event_to_av.diff

This is a second batch. Fixes browser_rules_search-filter* tests.

Comment 7

[Patrick Brosset <:pbro>]

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #3)
> ::: devtools/client/inspector/rules/test/head.js
> @@ +288,5 @@
> > + * ruleview-changed event.
> > + * @param {Tooltip} tooltip
> > + * @param {CSSRuleView} view
> > + */
> > +function* hideTooltipAndWaitForRuleviewChanged(tooltip, view) {
> 
> To stick to our naming conventions, we should capitalize the 'v' and rename
> the function to hideTooltipAndWaitForRuleViewChanged
I pushed this patch too quickly and forgot to do that change. I'll get it done in a later patch. Sorry about that.

Comment 8

[Patrick Brosset <:pbro>]

Attachment: Bug_1241527_-_3_-_Fix_typo_in_hideTooltipAndWaitFo.diff

Comment 9

[Patrick Brosset <:pbro>]

Attachment: Bug_1241527_-_4_-_Use_ruleview-changed_event_to_av.diff

One more ... Sorry for the review requests Gabriel, but these are all pretty small and straightforward changes (also, they're very similar).

Comment 10

[Patrick Brosset <:pbro>]

Part 2: https://treeherder.mozilla.org/#/jobs?repo=try&revision=533466d35756&group_state=expanded
Part 3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e06629945e2&group_state=expanded

Comment 11

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

Comment on attachment 8711026 [details] [diff] [review]
Bug_1241527_-_2_-_Use_ruleview-changed_event_to_av.diff

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

::: devtools/client/inspector/rules/test/browser_rules_search-filter_07.js
@@ +16,5 @@
>        height: 50%;
>      }
>    </style>
>    <h1 id='testid'>Styled Node</h1>
>  `;

We should keep the space to separate the variable and add_task()

@@ +21,5 @@
>  add_task(function*() {
>    yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
>    let {inspector, view} = yield openRuleView();
> +
> +  info("Select the test node #testid");

I think this info() is not needed since it is quite clear what is happening when reading: 
selectNode("#testid", inspector);

For these rule view tests, I try not to repeat myself if we can easily identify what is going on from the function name.

After looking at selectNode(), it also does an info() at the function definition as well.

::: devtools/client/inspector/rules/test/browser_rules_search-filter_08.js
@@ +22,5 @@
>  add_task(function*() {
>    yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
>    let {inspector, view} = yield openRuleView();
> +
> +  info("Select the test node #testid");

Remove info()

::: devtools/client/inspector/rules/test/browser_rules_search-filter_09.js
@@ +22,5 @@
>  add_task(function*() {
>    yield addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
>    let {inspector, view} = yield openRuleView();
> +
> +  info("Select the test node #testid");

Remove info()

@@ +52,4 @@
>    editor.input.value = "margin-left";
>  
>    info("Pressing return to commit and focus the new value field");
> +  let onModifications = view.once("ruleview-changed");

Rename onModifications to onRuleViewChanged to be consistent

Comment 12

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

Comment on attachment 8711034 [details] [diff] [review]
Bug_1241527_-_4_-_Use_ruleview-changed_event_to_av.diff

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

::: devtools/client/inspector/rules/test/browser_rules_multiple-properties-duplicates.js
@@ +17,2 @@
>    let ruleEditor = getRuleViewRuleEditor(view, 0);
> +  // Not that we wait for a markup mutation here because this new rule will end

I think you meant to write "Note" instead of "Not" here.

::: devtools/client/inspector/rules/test/browser_rules_multiple-properties-priority.js
@@ +17,2 @@
>    let ruleEditor = getRuleViewRuleEditor(view, 0);
> +  // Not that we wait for a markup mutation here because this new rule will end

Note*

::: devtools/client/inspector/rules/test/browser_rules_multiple-properties-unfinished_02.js
@@ +17,2 @@
>    let ruleEditor = getRuleViewRuleEditor(view, 0);
> +  // Not that we wait for a markup mutation here because this new rule will end

Note*

::: devtools/client/inspector/rules/test/browser_rules_multiple_properties_01.js
@@ +17,2 @@
>    let ruleEditor = getRuleViewRuleEditor(view, 0);
> +  // Not that we wait for a markup mutation here because this new rule will end

Note*

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4632b46da8a6

Comment 14

[Patrick Brosset <:pbro>]

(In reply to Gabriel Luong [:gl] (ΦωΦ) from comment #11)
> > +  info("Select the test node #testid");
> 
> I think this info() is not needed since it is quite clear what is happening
> when reading: 
> selectNode("#testid", inspector);
> 
> For these rule view tests, I try not to repeat myself if we can easily
> identify what is going on from the function name.
> 
> After looking at selectNode(), it also does an info() at the function
> definition as well.
My logic is: if it helps debugging test failures when reading the logs, then it's good. This sometimes means adding info("...") statements even if the code below them is straight forward. The target for these statements isn't the reader of the code, but for the person who's trying to understand test failures by reading logs.
But you're right, selectNode already has an info(), so I removed those I added.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/77907e30ffd7
https://hg.mozilla.org/integration/fx-team/rev/68b6b0e7b20d
https://hg.mozilla.org/integration/fx-team/rev/906f6d928a6d

Comment 16

[Patrick Brosset <:pbro>]

Here are the remaining ruleview tests that fail due to pending requests:
- browser_rules_editable-field-focus_02.js
- browser_rules_edit-property-increments.js
- browser_rules_edit-property_03.js
- browser_rules_edit-property_08.js
- browser_rules_editable-field-focus_01.js
- browser_rules_filtereditor-commit-on-ENTER.js

Comment 17

[Patrick Brosset <:pbro>]

Attachment: Bug_1241527_-_5_-_Use_ruleview-changed_event_to_fi.diff

This should be the last of them!
There are still exceptions in the logs I believe, but at least no more pending requests causing unhandled promise rejections.

Comment 18

[Patrick Brosset <:pbro>]

Try push for the part 5: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bef47bac84a4&group_state=expanded

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/77907e30ffd7
https://hg.mozilla.org/mozilla-central/rev/68b6b0e7b20d
https://hg.mozilla.org/mozilla-central/rev/906f6d928a6d

Comment 20

[Pulsebot]

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

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/158e783ff542