Closed Bug 885107 Opened 11 years ago Closed 11 years ago

testharness.js should deal with uncaught exceptions

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla27

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(2 files, 4 obsolete files)

I noticed this when I'm writing a test for bug 851982.

SimpleTest.expectAssertions() will throw ReferenceError in testharness tests because testharness won't define the global variable SimpleTest. Even worse, testharness driver didn't catch the failure. So the test will "pass" even if it is not actually executed.
I wouldn't say that bug 404077 broke testharness.js tests in general, but one test annotation done as part of bug 404077 broke a single test because the testharness.js wrapper fails to report toplevel exceptions, and thus I didn't notice that the annotation broke the test.  Or am I missing something here?
Summary: Bug 404077 effectively broke mozharness tests → Bug 404077 effectively broke testharness.js tests
Right, testharness.js should deal with toplevel exceptions.
They don't, by design.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
Then how can we detect silently disabled tests?
I think that design is unacceptable.  If it remains the case, I think we'll need to discourage people from writing testharness.js tests because of this issue.

The design aspect doesn't need to be applied to the W3C part; I'd think it could be in our extra integration script.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Perhaps testharnessreport.js?
Summary: Bug 404077 effectively broke testharness.js tests → Bug 404077 effectively broke testharnessreport.js tests
Summary: Bug 404077 effectively broke testharnessreport.js tests → testharnessreport.js should deal with uncaught exceptions
Yes, that's the file I meant by "extra integration script".
Depends on: 885765
Would it be enough if testharness.js installed an onerror handler that set the overall harness status to "error" if there was an uncaught exception? It could also report that in the visual output of course.

I am slightly worried that this will prove to be a non-backward-compatible change, but I can at least try and see if it breaks things.
I already tried and found that an existing imported test had a bug. (See bug 885765.)
This was not a test incompatibility but a proof of the ability to find broken tests.
OK, do you have a patch that you can turn into a PR at [1]?

[1] https://github.com/w3c/testharness.js
I took a safer approach (changed our integration script instead of testharness.js itself).
Assignee: nobody → VYV03354
Status: REOPENED → ASSIGNED
Attachment #771485 - Flags: review?(dbaron)
Work in progress patch for upstream:

https://github.com/jgraham/testharness.js/commit/caf73d4154144a7c2398ec07498ff11ef9b7aa96

Still needs documentation, but please try it out and report back any errors. You need to ensure that your testharnessreport.js checks the overall harness status as well as the individual test status.
(In reply to jgraham from comment #12)
> Work in progress patch for upstream:
> 
> https://github.com/jgraham/testharness.js/commit/
> caf73d4154144a7c2398ec07498ff11ef9b7aa96
> 
> Still needs documentation, but please try it out and report back any errors.
> You need to ensure that your testharnessreport.js checks the overall harness
> status as well as the individual test status.

It already does that.
testharness.js review request: https://critic.hoppipolla.co.uk/r/203
I tried to update testharness, but I got the following error:
https://tbpl.mozilla.org/php/getParsedLog.php?id=24997587#error0
"members.any" was "members.some" before the update.
updateTestharness.py points the following repo:
https://dvcs.w3.org/hg/resources
It has a fix for the above error:
https://dvcs.w3.org/hg/resources/diff/06cab30bcee2/idlharness.js
But this fix is not reflected to the GitHub repo.
On the other hand, dvcs.w3.org repo don't contain the windows.onerror things.

Which is the correct, latest, official testharness repo?
Flags: needinfo?(james)
Flags: needinfo?(ayg)
(In reply to Masatoshi Kimura [:emk] from comment #17)
> updateTestharness.py points the following repo:
> https://dvcs.w3.org/hg/resources
> It has a fix for the above error:
> https://dvcs.w3.org/hg/resources/diff/06cab30bcee2/idlharness.js
> But this fix is not reflected to the GitHub repo.
> On the other hand, dvcs.w3.org repo don't contain the windows.onerror things.
> 
> Which is the correct, latest, official testharness repo?

The github one. If there is a bug in that repo fixed in the old dvcs.w3.org one the patch needs to be ported across.
Flags: needinfo?(james)
Flags: needinfo?(ayg)
Pushed the patch for that bug.
Attachment #771485 - Attachment is obsolete: true
Attachment #771485 - Flags: review?(dbaron)
Attachment #773667 - Flags: review?(Ms2ger)
Attached patch Update known failures (obsolete) — Splinter Review
testharness update changes some failure messages.
Attachment #773668 - Flags: review?(Ms2ger)
Reverting testharnessreport.js changes because it is not needed anymore.
Attachment #773669 - Flags: review?(dbaron)
OS: Windows 8 → All
Hardware: x86_64 → All
Summary: testharnessreport.js should deal with uncaught exceptions → testharness.js should deal with uncaught exceptions
Ping?
Flags: needinfo?(Ms2ger)
Comment on attachment 773667 [details] [diff] [review]
Sync testharness with the latest upstream

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

::: dom/imptests/updateTestharness.py
@@ +11,5 @@
>  dest = "resources-upstream"
> +files = [{"f":"testharness.js"},
> +         {"f":"testharness.css"},
> +         {"f":"idlharness.js"},
> +         {"d":"webidl2/lib/webidl2.js", "f":"WebIDLParser.js"}]

These names are not terribly useful... I guess I can fix that later.
Attachment #773667 - Flags: review?(Ms2ger) → review+
Comment on attachment 773668 [details] [diff] [review]
Update known failures

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

r=me if this passes
Attachment #773668 - Flags: review?(Ms2ger) → review+
Flags: needinfo?(Ms2ger)
https://hg.mozilla.org/mozilla-central/rev/e12c6f7d6676
https://hg.mozilla.org/mozilla-central/rev/bf423f63b3a1
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Backed out on suspicion of causing bug 894952:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ffa345ca78
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c59a89e00478
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Will bug 895578 help?
Or bug 887903. I suppose this patch just exposed the underlying problem more frequently.
Depends on: 887903
Relanding only the test fix because it doesn't depend on the testharness update.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc10e63afa97
Whiteboard: [leave open]
Depends on: 901381
Bug 895578 and bug 887903 are fixed, but this patch will make bug 900633 very frequent now.
https://tbpl.mozilla.org/?tree=Try&rev=200c053e82c9
We really need to fix the underlying issue...
Depends on: 900633
I would not like to overlook exceptions thrown by testhaness tests until the OOM issue is resolved.
Attachment #785988 - Flags: review?(dbaron)
Fixing bug 859087 should fix the OOM issues on this set of tests.
Depends on: 859087
Comment on attachment 785988 [details] [diff] [review]
Add a global error handler to the testharness integration script

I guess this is ok  for now, but maybe leave a comment that we can remove this when we upgrade to the new version of testharness.js?

You've tested that this causes mochitests to fail, right?  (Both a TEST-UNEXPECTED-FAIL line and a failure of the test suite.)
Attachment #785988 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #37)
> I guess this is ok  for now, but maybe leave a comment that we can remove
> this when we upgrade to the new version of testharness.js?

Landed with a comment.

> You've tested that this causes mochitests to fail, right?  (Both a
> TEST-UNEXPECTED-FAIL line and a failure of the test suite.)

Yes, test update (already landed) was needed to fix errors caught by this patch.
Yet another proof we shouldn't ignore uncaught exceptions... :(
Depends on: 919313
Depends on: 920043
Bug 920064 change should be reverted once the new WebIDL2.js has been landed.
Depends on: 921298
- Folded the test update.
- Rebased to tip.
- Reverted the comment out added in bug 920064.
Requires the bug 921298 patch first.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=7b217f9ad22d
Attachment #773667 - Attachment is obsolete: true
Attachment #773668 - Attachment is obsolete: true
Attachment #785988 - Attachment is obsolete: true
Attachment #811230 - Flags: review?(Ms2ger)
Attachment #811230 - Flags: feedback?(sheriffs)
What kind of feedback are you looking for?
Flags: in-testsuite+
Target Milestone: mozilla25 → ---
Confirming that the patch will not cause random orange again?
Comment on attachment 811230 [details] [diff] [review]
Sync testharness with the latest upstream

If Try is happy, I'm happy. We're not expecting any issues on other platforms I take it?
Attachment #811230 - Flags: feedback?(sheriffs) → feedback+
Comment on attachment 811230 [details] [diff] [review]
Sync testharness with the latest upstream

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

rs=me, assuming you checked that

/dom/imptests/html/dom/test_interfaces.html
/dom/imptests/editing/selecttest/test_interfaces.html
/dom/imptests/webapps/XMLHttpRequest/tests/submissions/Ms2ger/test_interfaces.html

all still run.
Attachment #811230 - Flags: review?(Ms2ger) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/75b92ed22e1a
Status: REOPENED → ASSIGNED
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/75b92ed22e1a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: