testharness.js should deal with uncaught exceptions

RESOLVED FIXED in mozilla27

Status

defect
RESOLVED FIXED
6 years ago
6 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

6 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

6 years ago
Right, testharness.js should deal with toplevel exceptions.
They don't, by design.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Assignee

Comment 4

6 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

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

Updated

6 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

6 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

6 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

6 years ago
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

6 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

6 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

6 years ago
Attachment #771485 - Attachment is obsolete: true
Attachment #771485 - Flags: review?(dbaron)
Attachment #773667 - Flags: review?(Ms2ger)
Assignee

Comment 21

6 years ago
Posted patch Update known failures (obsolete) β€” β€” Splinter Review
testharness update changes some failure messages.
Attachment #773668 - Flags: review?(Ms2ger)
Assignee

Comment 22

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

Updated

6 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

6 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
Closed: 6 years ago6 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

6 years ago
Will bug 895578 help?
Assignee

Comment 31

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

Updated

6 years ago
Depends on: 887903
Assignee

Comment 32

6 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

6 years ago
Depends on: 901381
Assignee

Comment 34

6 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

6 years ago
Depends on: 900633
Assignee

Comment 35

6 years ago
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

6 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

6 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

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

Updated

6 years ago
Depends on: 920043
Assignee

Comment 42

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

Updated

6 years ago
Depends on: 921298
Assignee

Comment 43

6 years ago
- 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

6 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

6 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
Closed: 6 years ago6 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.