1513577 - Add selftest for manifest parsing

Status: RESOLVED FIXED

(Testing :: Reftest, enhancement)

Description

[Andrew Halberstadt [:ahal]]

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

Comment 1

[Andrew Halberstadt [:ahal]]

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.

Comment 2

[Andrew Halberstadt [:ahal]]

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.

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1513577 - [reftest] Update selftests with new local specialpowers location, r?jmaher

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.

Comment 4

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1513577 - [reftest] Refactor the runtests fixture to a more re-useable 'get_reftest' fixture, r?jmaher

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

Comment 5

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1513577 - [reftest] Add a selftest for the manifest parser, r?jmaher

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

Comment 6

[Andrew Halberstadt [:ahal]]

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.

Comment 7

[Andrew Halberstadt [:ahal]]

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)

Comment 8

[Andrew Halberstadt [:ahal]]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Andrei Ciure[:aciure]]

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

Comment 11

[André Bargull [:anba]]

(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

Comment 12

[Andrew Halberstadt [:ahal]]

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