In bug 1392391 I introduced a bad regression to the manifest parsing logic. This caused 'script' reftests to be treated as 'load' reftests. It was able to land because the script tests would time out and print a message that wasn't caught by the treeherder log parser. This was finally caught and fixed in bug 1512989. I should have written a better test for the manifestparser before landing that original patch, but let's at least get one landed now. We should also hunt down the message that got logged when the script tests timed out and make sure it is structured (so the job turns orange).

Here's an example log that happened when a "script" test is treated as a "load" (taken from bug 1512989 comment 0): [task 2018-12-10T14:23:43.892Z] 14:23:43 INFO - REFTEST TEST-START | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=non262/regress/regress-1476383-calloc-exc.js [task 2018-12-10T14:23:43.893Z] 14:23:43 INFO - REFTEST TEST-LOAD | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=non262/regress/regress-1476383-calloc-exc.js | 1959 / 11236 (17%) [task 2018-12-10T14:23:43.944Z] 14:23:43 INFO - TEST-INFO | FAILED! ReferenceError: quit is not defined [task 2018-12-10T14:23:43.945Z] 14:23:43 INFO - JavaScript error: file:///builds/worker/workspace/build/tests/jsreftest/tests/non262/regress/regress-1476383-calloc-exc.js, line 3: ReferenceError: quit is not defined [task 2018-12-10T14:23:43.970Z] 14:23:43 INFO - REFTEST TEST-PASS | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=non262/regress/regress-1476383-calloc-exc.js | (LOAD ONLY) [task 2018-12-10T14:23:43.970Z] 14:23:43 INFO - REFTEST TEST-END | file:///builds/worker/workspace/build/tests/jsreftest/tests/jsreftest.html?test=non262/regress/regress-1476383-calloc-exc.js Looks like "script" tests have globals injected into their scope not available to load, and the resulting ReferenceError doesn't turn the job orange. If it had, we likely would have had some sort of timeout message that would have turned the job orange.

I came up with a test that fails without the fix in bug 1512989 and passes afterwards. I'll wait for that to land before landing it though.

This was a regression from bug 1497339 which changed the location of specialpowers in the objdir. While the reftest harness itself was changed, the selftests weren't updated. Only the local case was affected since the location of specialpowers in the tests.zip remained the same. This explains why this wasn't caught in CI.

This will make it easier for new tests to get a handle on a RefTest instance so they can do a wider variety of unittesting. Depends on D14300

This test would have caught the regression detected and fixed in bug 1512989. It also sets up the scaffolding to add future tests to the reftest manifestparser. Depends on D14301

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46cdc904566a379291a4af70e2e1a4471840e054 The failure is expected (it's the regression that gets fixed by bug 1512989). Once that bug is merged, I'll re-push to verify everything is green.

Hi André, I've been trying to track down why that ReferenceError didn't turn the job orange. As far as I can tell, it looks like it gets logged from here: https://searchfox.org/mozilla-central/source/js/src/tests/browser.js#374 It looks like that call to `new TestCase(...)` above *should* cause the reftest harness to log a TEST-FAIL, as long as the `expectedError` error variable isn't "error". Do you know if we are expecting an "error" here? If so, how can we make sure unexpected errors like that ReferenceError cause a failure? (This is my first time looking at the jstest harness, and I'm struggling to follow along a little)

Try run on top of bug 1512989 looks good so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8ff690c59919b46103edc51f9c1f3a0d634f9b6

Pushed by ahalberstadt@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/06285a5e6d09 [reftest] Update selftests with new local specialpowers location, r=jmaher https://hg.mozilla.org/integration/autoland/rev/6cd9b1c5306e [reftest] Refactor the runtests fixture to a more re-useable 'get_reftest' fixture, r=jmaher https://hg.mozilla.org/integration/autoland/rev/05b132dec73b [reftest] Add a selftest for the manifest parser, r=jmaher

https://hg.mozilla.org/mozilla-central/rev/06285a5e6d09 https://hg.mozilla.org/mozilla-central/rev/6cd9b1c5306e https://hg.mozilla.org/mozilla-central/rev/05b132dec73b

(In reply to Andrew Halberstadt [:ahal] from comment #7) > It looks like that call to `new TestCase(...)` above *should* cause the > reftest harness to log a TEST-FAIL, as long as the `expectedError` error > variable isn't "error". Do you know if we are expecting an "error" here? If > so, how can we make sure unexpected errors like that ReferenceError cause a > failure? AFAIK the browser jsreftest error handling and reporting works as follows: - |window.onerror| gets called for unhandled exceptions. - |window.onerror| then calls |new TestCase| to report the exception, with additional steps to see if the unhandled exception was expected. - The |reportFailure| call in |window.onerror| is irrelevant for the test reporter, it just adds a <div> to the test document to aid manual test inspection of any test failures. (*) - The |TestCase| constructor then sets the |passed| field, depending on the result of |getTestCaseResult|. - Additionally the |TestCase| constructor adds the new TestCase object to the |testCasesArray| list [2]. - The |testCasesArray| list is exposed via |getTestCases| for reftest-content.js [4]. - But reftest-content.js only inspects |getTestCases| for TYPE_SCRIPT tests [5], so any failures, like a ReferenceError, for TYPE_LOAD is simply ignored. (*) |reportFailure| calls this |print| function [6], which in turn calls this |AddPrintOutput| function [7]. And |AddPrintOutput| appends the print message to the "jsreftest-print-output-container" <div> in [8]. Additionally |print| also calls |dump| [9], which can be a pre-existing |dump| function [10]. Maybe this is how the "INFO - TEST-INFO | FAILED! ReferenceError: quit is not defined" output ended up in the log? [1] https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/js/src/tests/shell.js#327,331 [2] https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/js/src/tests/shell.js#325 [3] https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/js/src/tests/shell.js#383-386 [4] https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/layout/tools/reftest/reftest-content.js#1008 [5] https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/layout/tools/reftest/reftest-content.js#996 [6] https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/js/src/tests/shell.js#204-217 [7] https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/js/src/tests/browser.js#312-319 [8] https://searchfox.org/mozilla-central/source/js/src/tests/jsreftest.html#18 [9] https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/js/src/tests/shell.js#213 [10] https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/js/src/tests/shell.js#183-200

(In reply to André Bargull [:anba] from comment #11) > - But reftest-content.js only inspects |getTestCases| for TYPE_SCRIPT tests > [5], so any failures, like a ReferenceError, for TYPE_LOAD is simply ignored. Oh right, thanks! This was the piece I was missing. In that case I guess everything was working as expected. I suppose we *could* inspect `getTestCases` for non-script types, but since there's now a test that should prevent this from happening in the future, it's probably not worth the effort.