testharness.js should deal with uncaught exceptions

RESOLVED FIXED in mozilla27

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

unspecified
mozilla27
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Comment 2

5 years ago
Right, testharness.js should deal with toplevel exceptions.
They don't, by design.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 4

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

Comment 6

5 years ago
Perhaps testharnessreport.js?
Summary: Bug 404077 effectively broke testharness.js tests → Bug 404077 effectively broke testharnessreport.js tests
(Assignee)

Updated

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

Updated

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

Comment 9

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

Comment 11

5 years ago
Created attachment 771485 [details] [diff] [review]
Add a global error handler to the testharness integration script

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
(Assignee)

Comment 16

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

Comment 17

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

Comment 20

5 years ago
Created attachment 773667 [details] [diff] [review]
Sync testharness with the latest upstream
Attachment #771485 - Attachment is obsolete: true
Attachment #771485 - Flags: review?(dbaron)
Attachment #773667 - Flags: review?(Ms2ger)
(Assignee)

Comment 21

5 years ago
Created attachment 773668 [details] [diff] [review]
Update known failures

testharness update changes some failure messages.
Attachment #773668 - Flags: review?(Ms2ger)
(Assignee)

Comment 22

5 years ago
Created attachment 773669 [details] [diff] [review]
Fix testharness tests outside dom/imptests

Reverting testharnessreport.js changes because it is not needed anymore.
Attachment #773669 - Flags: review?(dbaron)
Attachment #773669 - Flags: review?(dbaron) → review+
(Assignee)

Updated

5 years ago
OS: Windows 8 → All
Hardware: x86_64 → All
Summary: testharnessreport.js should deal with uncaught exceptions → testharness.js should deal with uncaught exceptions
(Assignee)

Comment 24

5 years ago
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
Last Resolved: 5 years ago5 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 → ---
(Assignee)

Comment 30

5 years ago
Will bug 895578 help?
(Assignee)

Comment 31

5 years ago
Or bug 887903. I suppose this patch just exposed the underlying problem more frequently.
(Assignee)

Updated

5 years ago
Depends on: 887903
(Assignee)

Comment 32

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

Updated

5 years ago
Depends on: 901381
(Assignee)

Comment 34

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

Updated

5 years ago
Depends on: 900633
(Assignee)

Comment 35

5 years ago
Created attachment 785988 [details] [diff] [review]
Add a global error handler to the testharness integration script

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

Updated

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

Comment 39

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

Comment 41

5 years ago
Yet another proof we shouldn't ignore uncaught exceptions... :(
Depends on: 919313
(Assignee)

Updated

5 years ago
Depends on: 920043
(Assignee)

Comment 42

5 years ago
Bug 920064 change should be reverted once the new WebIDL2.js has been landed.
(Assignee)

Updated

5 years ago
Depends on: 921298
(Assignee)

Comment 43

5 years ago
Created attachment 811230 [details] [diff] [review]
Sync testharness with the latest upstream

- 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 → ---
(Assignee)

Comment 45

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

Comment 48

5 years ago
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
Last Resolved: 5 years ago5 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.