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?

Right, testharness.js should deal with toplevel exceptions.

They don't, by design.

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.

Perhaps testharnessreport.js?

Yes, that's the file I meant by "extra integration script".

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

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

trstharness.js patch pushed: https://github.com/w3c/testharness.js/commit/103f3c0

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?

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

Pushed the patch for that bug.

testharness update changes some failure messages.

Reverting testharnessreport.js changes because it is not needed anymore.

Ping?

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.

Comment on attachment 773668 [details] [diff] [review] Update known failures Review of attachment 773668 [details] [diff] [review]: ----------------------------------------------------------------- r=me if this passes

https://hg.mozilla.org/integration/mozilla-inbound/rev/e12c6f7d6676 https://hg.mozilla.org/integration/mozilla-inbound/rev/bf423f63b3a1

https://hg.mozilla.org/mozilla-central/rev/e12c6f7d6676 https://hg.mozilla.org/mozilla-central/rev/bf423f63b3a1

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

Will bug 895578 help?

Or bug 887903. I suppose this patch just exposed the underlying problem more frequently.

Relanding only the test fix because it doesn't depend on the testharness update. https://hg.mozilla.org/integration/mozilla-inbound/rev/bc10e63afa97

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...

I would not like to overlook exceptions thrown by testhaness tests until the OOM issue is resolved.

Fixing bug 859087 should fix the OOM issues on this set of tests.

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

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

Backed out for mochitest failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb253f53548 https://tbpl.mozilla.org/php/getParsedLog.php?id=26263236&tree=Mozilla-Inbound

Yet another proof we shouldn't ignore uncaught exceptions... :(

Bug 920064 change should be reverted once the new WebIDL2.js has been landed.

- 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

What kind of feedback are you looking for?

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?

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.