Fix remaining unhandled rejected promises in rule-view mochitests

RESOLVED FIXED in Firefox 47

Status

()

Firefox
Developer Tools: CSS Rules Inspector
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: pbro, Assigned: pbro)

Tracking

unspecified
Firefox 47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
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.
(Assignee)

Comment 2

2 years ago
Created attachment 8710509 [details] [diff] [review]
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.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
Attachment #8710509 - Flags: review?(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
Attachment #8710509 - Flags: review?(gl) → review+
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 6

2 years ago
Created attachment 8711026 [details] [diff] [review]
Bug_1241527_-_2_-_Use_ruleview-changed_event_to_av.diff

This is a second batch. Fixes browser_rules_search-filter* tests.
Attachment #8711026 - Flags: review?(gl)
(Assignee)

Comment 7

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8710509 - Flags: checkin+
(Assignee)

Comment 8

2 years ago
Created attachment 8711027 [details] [diff] [review]
Bug_1241527_-_3_-_Fix_typo_in_hideTooltipAndWaitFo.diff
Attachment #8711027 - Flags: review?(gl)
(Assignee)

Comment 9

2 years ago
Created attachment 8711034 [details] [diff] [review]
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).
Attachment #8711034 - Flags: review?(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
Attachment #8711026 - Flags: review?(gl) → review+

Updated

2 years ago
Attachment #8711027 - Flags: review?(gl) → review+
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*
Attachment #8711034 - Flags: review?(gl) → review+
(Assignee)

Comment 14

2 years ago
(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.
(Assignee)

Updated

2 years ago
Attachment #8711026 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8711027 - Flags: checkin+
(Assignee)

Updated

2 years ago
Attachment #8711034 - Flags: checkin+
(Assignee)

Comment 16

2 years ago
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
(Assignee)

Comment 17

2 years ago
Created attachment 8711681 [details] [diff] [review]
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.
Attachment #8711681 - Flags: review?(gl)

Updated

2 years ago
Attachment #8711681 - Flags: review?(gl) → review+
(Assignee)

Updated

2 years ago
Keywords: leave-open

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/158e783ff542
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.