610046 - Let jsreftests that should silently fail do so

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[Alon Zakai (:azakai)]

See

https://bugzilla.mozilla.org/show_bug.cgi?id=502836#c19

A serious error is encountered, the script is silently halted, but the test harness gets no indication of anything, so it thinks the script finished normally (in the case above, without running necessary code at the end of the script, so the test fails).

The intended behavior, as explained by igor, is that the error should not be fatal - the browser should not quit - but the script should be halted, in hopes of recovering memory from that. So that is ok; all that is wrong is that the error is not detected by the test harness.

So, such errors should be 'caught' by the test harness, so that expectExitCode will work.

Comment 1

[Alon Zakai (:azakai)]

Requesting blocking as this is needed for bug 502836 (which is blocking).

Comment 2

[Alon Zakai (:azakai)]

Attachment: patch

Patch allows reftests (and in particular jsreftests) to be explicitly marked as allowing silent failure (with |silentfail|). On such a test, if no test results were reported, we mark it as having passed.

This allows enabling a few jsreftests that were disabled before, 336409-2, 336410-1, 336410-2, all of which had issues when running in the browser, due to triggering out of memory situations (on certain build types), which are purposefully silently failed on (in the browser; the js shell will terminate itself). It also allows the patch to blocker bug 502836 to be pushed (after which we can run into out of memory situations in some jsreftests as well, because we allow more system memory to be used, so when we run out of memory can change).

P.S. I'm not sure whether this patch needs blocking status in order to be pushed, as it's just changes to test code? (I presume reftest.js is only run when running reftests?)

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 489283 [details] [diff] [review]
patch

Please add documentation of the new keyword to layout/tools/reftest/README.txt

>             } else if ((m = item.match(/^slow-if\((.*?)\)$/))) {
>                 cond = false;
>                 if (Components.utils.evalInSandbox("(" + m[1] + ")", sandbox))
>                     slow = true;
>+            } else if (item == "silentfail") {
>+                allow_silent_fail = true;
>             } else {
>                 throw "Error 1 in manifest file " + aURL.spec + " line " + lineNo;
>             }

Two issues here:
 1) you should also add support for silentfail-if, to match the pattern of the other keywords.
 2) you should set cond = false; otherwise you'll reuse cond from the previous iteration of the loop, which could be wrong

Other than that, this looks fine, but :bc: should also review this (in addition to me).

Comment 4

[Bob Clary [:bc] (inactive)]

>         else if (testcases.length == 0) {
>             // This failure may be due to a JavaScript Engine bug causing
>-            // early termination of the test.
>-            missing_msg = "No test results reported. (SCRIPT)\n";
>+            // early termination of the test. If we do not allow silent
>+            // failure, report an error.
>+            if (!gURLs[0].allowSilentFail)
>+                missing_msg = "No test results reported. (SCRIPT)\n";

Do we want to output an INFO type message if the error is suppressed due to allowSilentFail?

Comment 5

[Alon Zakai (:azakai)]

Attachment: patch v2

Ok, here's a patch that fixes the issues that were mentioned.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 489369 [details] [diff] [review]
patch v2

Looks good, though you could actually also handle silentfail by just adding to the regex in:

            } else if (item.match(/^(fails|random|skip)$/)) {
                stat = item;
                cond = true;

instead of adding:

+            } else if (item == "silentfail") {
+                cond = false;
+                allow_silent_fail = true;

(And we could probably handle slow the same way; that would have been cleaner.  But that's another patch...)


Also, the documentation in README.txt should mention that this is only for script tests.

r=dbaron with that.

Comment 7

[Alon Zakai (:azakai)]

Attachment: patch v3

I was overly optimistic in enabling those old tests - they fail intermittently. So, I re-disabled them, in this patch the only change to tests is to add |silentfail| where necessary, nothing else.

Comment 8

[Alon Zakai (:azakai)]

Comment on attachment 491217 [details] [diff] [review]
patch v3

Not sure if this change (re-enabling the old tests that the previous patch enabled) needs a review or not?

Comment 9

[Alon Zakai (:azakai)]

http://hg.mozilla.org/tracemonkey/rev/b12f0464da32

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/b12f0464da32

Comment 11

[Paul Biggar]

This changeset adds the following warnings in the js shell tests:

warning: invalid manifest line element "silentfail"
warning: invalid manifest line element "silentfail"
warning: invalid manifest line element "silentfail"


Are they benign?

Comment 12

[Alon Zakai (:azakai)]

Hmm, we only really need the ability to ignore silent fails in the browser, so these are fairly harmless, but maybe we should clean them up anyhow. Which option is better?

1) Handle silent failure in shell tests (even though we don't need it, but for consistency)
2) Ignore 'silentfail' so that warning is not shown.

Comment 13

[Paul Biggar]

What would 1) entail?

Comment 14

[Alon Zakai (:azakai)]

I would guess something similar to what the browser ones have - if there is no log output about tests passing (which can happen if a test silently halts, in the browser), then consider that ok if |silentfail| was set. But we'd need to figure out whether such silent failures are even possible in the shell and how they behave.

Comment 15

[Paul Biggar]

I would guess we should make it the same as 'skip'. I don't see the benefit of running the test to ignore it's failure. I don't fully understand why the browser does it, but I think those reasons don't apply when testing in the shell?

Comment 16

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

One benefit is that we still test that the test doesn't crash.

Comment 17

[Paul Biggar]

I'd go with that, except that these tests use tons of memory, and might be better skipped anyway?

Comment 18

[Paul Biggar]

I took care of my follow-up comments (js shell changes) in bug 620850.