Closed Bug 1016387 Opened 10 years ago Closed 10 years ago

Uncaught async Promise errors should cause mochi tests to fail

Categories

(Toolkit :: Async Tooling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: Yoric, Assigned: Yoric)

References

(Depends on 3 open bugs, Blocks 1 open bug)

Details

Attachments

(11 files, 6 obsolete files)

1.44 KB, patch
Details | Diff | Splinter Review
4.21 KB, patch
ted
: review+
Details | Diff | Splinter Review
5.37 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
1.04 KB, patch
mossop
: review+
Details | Diff | Splinter Review
38.06 KB, patch
paul
: review+
Details | Diff | Splinter Review
910 bytes, patch
Yoric
: review+
Details | Diff | Splinter Review
7.15 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
1.14 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
1.36 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
855 bytes, patch
Yoric
: review+
Details | Diff | Splinter Review
2.13 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
      No description provided.
Same patch with less line noise in the message.
Attachment #8431052 - Attachment is obsolete: true
No longer depends on: 985413
No longer depends on: 985417
No longer depends on: 985423
No longer depends on: 985424
No longer depends on: 985427
Unfortunately, the blockers for this bug have actually regressed in the past few months. I'll change strategy and whitelist offenders.
Attached patch 2. Whitelist mechanism (obsolete) — Splinter Review
Attachment #8499538 - Flags: review?(ted)
Whitelisting the offenders from mochitest-bc1. Gijs, you have been picked as reviewer :)
Attachment #8499542 - Flags: review?(gijskruitbosch+bugs)
Dao, this one-liner is for you.
Attachment #8499550 - Flags: review?(dao)
Comment on attachment 8499542 [details] [diff] [review]
3. Whitelisting offenders from bc1

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

Please fix the trailing whitespace.
Attachment #8499542 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8499550 [details] [diff] [review]
4. Whitelisting offenders from bc3

I don't care much since it's a test, but the empty comment lines are weird.
Attachment #8499550 - Flags: review?(dao) → review+
Comment on attachment 8499538 [details] [diff] [review]
2. Whitelist mechanism

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

::: testing/mochitest/browser-test.js
@@ +907,5 @@
>      self.SimpleTest.ignoreAllUncaughtExceptions(aIgnoring);
>    };
>  
> +  this.thisTestLeaksUncaughtRejectionsAndShouldBeFixed = function() {
> +    aTester._treatUncaughtRejectionsAsFailures = false;

I feel like these ought to wind up in the manifest, but that's probably not as straightforward.
Attachment #8499538 - Flags: review?(ted) → review+
As discussed over IRC, here is a more explicit v2.
Attachment #8499538 - Attachment is obsolete: true
Attachment #8500050 - Flags: review?(ted)
Applied feedback.
Attachment #8499542 - Attachment is obsolete: true
Attachment #8500052 - Flags: review+
Applied feedback.
Attachment #8499550 - Attachment is obsolete: true
Attachment #8500053 - Flags: review+
Comment on attachment 8500053 [details] [diff] [review]
4. Whitelisting offenders from bc3, v2

>+///////////////////
>+//
>+// Whitelisting this test.
>+// As part of bug 1077403, the leaking uncaught rejection should be fixed.
>+//
>+thisTestLeaksUncaughtRejectionsAndShouldBeFixed("");

That's even worse. Should be like this:

// Whitelisting this test.
// As part of bug 1077403, the leaking uncaught rejection should be fixed.
thisTestLeaksUncaughtRejectionsAndShouldBeFixed("");

Though since the verbose function name says it all, "Whitelisting this test" could also be spared...
And here comes the main course: offenders from devtools. Paul, I'd still be grateful if all of these errors could be fixed instead of being whitelisted.
Attachment #8500055 - Flags: review?(paul)
As you wish. I just wanted to make sure that whoever maintains the test cannot miss that single line, because in my experience, most of the uncaught errors represent actual bugs.
Attachment #8500053 - Attachment is obsolete: true
Attachment #8500056 - Flags: review+
Attachment #8500055 - Flags: review?(paul) → review+
Attachment #8500054 - Flags: review?(dtownsend+bugmail) → review+
Attachment #8500050 - Flags: review?(ted) → review+
Let's try and land this.
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Tomcat, according to comment 19, you only landed 4 patches out of 7. This is consistent with the error message in the logs: part 2 is missing.
Ah, apparently, one more test needs to be whitelisted.
Attachment #8503084 - Flags: review+
Hopefully, this should take care of all the devtools oranges that have appeared since my last Try.
Attachment #8503084 - Attachment is obsolete: true
Attachment #8503090 - Flags: review+
And another new one, probably intermittent.
Attachment #8503092 - Flags: review+
Attachment #8503119 - Attachment is patch: true
Attachment #8503119 - Flags: review+
Attachment #8503119 - Attachment description: 19. Whitelisting offenders from bc1, part 3 → 10. Whitelisting offenders from bc1, part 3
Depends on: 1081547
Depends on: 1081548
Depends on: 1081549
Depends on: 1081551
Depends on: 1082628
Depends on: 1081345
Depends on: 1085604
Depends on: 1086533
Depends on: 1086909
Depends on: 1085883
Depends on: 1087110
Depends on: 1088725
Depends on: 1089849
Depends on: 1090539
Depends on: 1094329
Flags: qe-verify-
No longer depends on: 1086533
You need to log in before you can comment on or make changes to this bug.