Unhandled promise rejections in rule view tests (round 2)

RESOLVED FIXED in Firefox 47

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: jryans, Assigned: pbro)

Tracking

Trunk
Firefox 47
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 years ago
I have done an updated try run to attempt enabling promise rejection tracking in bug 1240804.  There still appear to be some unresolved rule view failures:

devtools/client/inspector/rules/test/browser_rules_add-property_01.js
devtools/client/inspector/rules/test/browser_rules_colorpicker-commit-on-ENTER.js
devtools/client/inspector/rules/test/browser_rules_colorpicker-hides-on-tooltip.js
devtools/client/inspector/rules/test/browser_rules_colorpicker-revert-on-ESC.js
devtools/client/inspector/rules/test/browser_rules_completion-existing-property_02.js
devtools/client/inspector/rules/test/browser_rules_completion-new-property_02.js
devtools/client/inspector/rules/test/browser_rules_completion-new-property_04.js
devtools/client/inspector/rules/test/browser_rules_cubicbezier-revert-on-ESC.js
devtools/client/inspector/rules/test/browser_rules_cubicbezier-revert-on-ESC.js
devtools/client/inspector/rules/test/browser_rules_edit-property_04.js
devtools/client/inspector/rules/test/browser_rules_edit-property_04.js
devtools/client/inspector/rules/test/browser_rules_edit-property-cancel.js
devtools/client/inspector/rules/test/browser_rules_edit-property-cancel.js
devtools/client/inspector/rules/test/browser_rules_edit-property-commit.js
devtools/client/inspector/rules/test/browser_rules_edit-property-commit.js
devtools/client/inspector/rules/test/browser_rules_eyedropper.js
devtools/client/inspector/rules/test/browser_rules_eyedropper.js
devtools/client/inspector/rules/test/browser_rules_livepreview.js
devtools/client/inspector/rules/test/browser_rules_livepreview.js
devtools/client/inspector/rules/test/browser_rules_livepreview.js
devtools/client/inspector/rules/test/browser_rules_multiple_properties_02.js
devtools/client/inspector/rules/test/browser_rules_multiple_properties_02.js
devtools/client/inspector/rules/test/browser_rules_multiple_properties_02.js

Remember to apply the patch from bug 1240804 to see the failures.

More details available at:

https://docs.google.com/spreadsheets/d/1ZKXW9HqnxJNdeEVG7s33_TGSV94PdUJCsQsZe1GGqjY/edit
Assignee

Comment 1

3 years ago
Working on massive patch series in bug 1246677 that aims at getting rid of CPOW usage in rule-view tests.
I ended up cleaning more in that bug that just CPOW usages, including getting rid of some promise rejections.
Depends on: 1246677
Assignee

Comment 2

3 years ago
If I apply the patches from bug 1246677 *and* run each test from comment 0 individually, with e10s, then only the following ones still fail:

devtools/client/inspector/rules/test/browser_rules_completion-existing-property_02.js
devtools/client/inspector/rules/test/browser_rules_completion-new-property_02.js
devtools/client/inspector/rules/test/browser_rules_edit-property-commit.js
devtools/client/inspector/rules/test/browser_rules_multiple_properties_02.js

They should be pretty easy to fix.
I still need to run the whole test suite though as  that might impact the result, and try without e10s too.
Assignee

Comment 4

3 years ago
Sorry jryans for picking you for this, feel free to forward to someone else if you don't want to review code changes in this part of the codebase. Hopefully this is simple enough (and test-only).
The basic thing here is waiting for the right events at the right time ("ruleview-changed").
I have already inundated my usual go-to reviewers for the rule-view in bug 1246677
Reporter

Comment 5

3 years ago
Comment on attachment 8718892 [details]
MozReview Request: Bug 1247263 - Round 2 of fixing unhandled rejected promises in ruleview tests

:tromey said he'll take a look at this.
Attachment #8718892 - Flags: review?(jryans) → review?(ttromey)
Comment on attachment 8718892 [details]
MozReview Request: Bug 1247263 - Round 2 of fixing unhandled rejected promises in ruleview tests

https://reviewboard.mozilla.org/r/34785/#review31421

Looks great, thanks for doing this.
Attachment #8718892 - Flags: review?(ttromey) → review+
Assignee

Comment 7

3 years ago
Sorry for the delay here. I know this is R+ but it depends on bug 1246677 which itself depending on another bug that took time to land. I finally landed bug 1246677 but it got backed out. I'm working on a fix though.
In the meantime, I have a green try for the changes here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13dc6c29fb82&group_state=expanded
But pushing to try with the patch in bug 1240804 applied (to fail on unhandled promise rejections) still shows 3 failures in rule-view: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc2024973bc1&group_state=expanded
Everything was passing fine locally last time I tried. So I'll run these again and try and fix the remaining issues.
Assignee

Comment 8

3 years ago
I was able to fix 2 of the 3 remaining failures. I can't reproduce the last one locally.
So, since this patch has been around for some time, is R+ and is green on try, I will land it as is, and we can file a new bug to fix the remaining failures with bug 1240804 applied, if any.

Comment 11

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a96f17e627e5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.