498685 - Add isDebugBuild to reftest sandbox

Status: RESOLVED FIXED

(Testing :: Reftest, enhancement)

Description

[Bob Clary [:bc] (inactive)]

If reftest manifest *-if rules allowed the use of the debug status of a build, it would be possible to skip tests with fatal assertions in debug builds while allowing the tests to be run in opt builds.

Ted pointed out to me that nsIDebug2.idl is already used in reftest.js to create the global variable gDebug and that gDebug.isDebugBuild is already available.

Comment 1

[Bob Clary [:bc] (inactive)]

Attachment: patch

David, this adds isDebugBuild to the sandbox. It also modifies the manifest parser to allow fail to be specified with load. That is necessary if the reftest framework is to be used to run the javascript tests in the browser. I added error numbers to the various manifest parser errors as the identical error messages made it difficult to determine why a particular manifest error occurred.

Comment 2

[Bob Clary [:bc] (inactive)]

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

Sorry, dbaron alone wasn't sufficient as the requestee.

FYI, I tested the following manifest line with opt and debug with this patch and it worked for me.

skip-if(isDebugBuild) fails-if(!isDebugBuild) load ../../jsreftest.html?test=js1_8/regress/regress-455981-02.js

Comment 3

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

Why do you want 'fail' with 'load'?

Comment 4

[Bob Clary [:bc] (inactive)]

fail would allow me to run a test which is not expected to crash, hang or time out but which fails due to a comparison failure. It would allow the detection of fatal js assertions or other crashes which might occur due to a regression.

Without fail, I would have to skip the non fatal failures which would reduce the test coverage.

This is part of the idea to get either the reftest framework to communicate with the test and issue the appropriate TEST-KNOWN-FAIL or TEST-UNEXPECTED-FAIL messages for the individual test results or for the individual tests to do so as I mentioned in my email. It would be much simpler, for me anyway, if the reftests took that responsibility. If this idea is in principle ok with you, I can work up a patch including an implementation for review. If it is better to do so, I can do that in a different bug.

Comment 5

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

(In reply to comment #4)
> fail would allow me to run a test which is not expected to crash, hang or time
> out but which fails due to a comparison failure. It would allow the detection

What comparison?  There's no comparison done with 'load'.

Comment 6

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

(In reply to comment #4)
> This is part of the idea to get either the reftest framework to communicate
> with the test and issue the appropriate TEST-KNOWN-FAIL or TEST-UNEXPECTED-FAIL
> messages for the individual test results or for the individual tests to do so
> as I mentioned in my email. It would be much simpler, for me anyway, if the

This sounds like something more appropriate for mochitest than reftest; in fact, it sounds a lot like what mochitest already does.

Comment 7

[Bob Clary [:bc] (inactive)]

I'm trying to leverage the reftest framework to run the existing javascript tests in the browser without having to reinvent the wheel or convert the javascript tests to mochitests.

Comment 8

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

Wouldn't it be easier to add a wrapper page that wraps the JS tests in mochitest calls than a wrapper page that wraps the JS tests in a new reftest API that doesn't exist yet?

Comment 9

[Bob Clary [:bc] (inactive)]

Not really. I already have the js tests running under the reftest framework in the wip patch in bug 469718. The lacking parts in the reftest frame work are:

1. ability to use 'fails' with 'load'. 
2. ability to distinguish debug from non-debug builds.
3. the ability to distinguish known from unexpected failures.

1 & 2 are fixed with this patch. I'm looking at a straw-man patch for 3 at the moment.

mochitests don't have the flexibility of the reftest-style manifests to include or exclude tests based on platform, or build type which I would dearly love. I suppose it would be possible to wrap mochitests around the js tests, but I don't think it would be the best choice.

Comment 10

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

(In reply to comment #9)
> Not really. I already have the js tests running under the reftest framework in
> the wip patch in bug 469718. The lacking parts in the reftest frame work are:
> 
> 1. ability to use 'fails' with 'load'. 

I don't see how this is useful without something else.

> 2. ability to distinguish debug from non-debug builds.
> 3. the ability to distinguish known from unexpected failures.

reftest already has this; it's what the "fails" keyword is for.



A few questions about the JS tests:
 * do the tests report pass or fail, or are they simply "not crash" tests?
 * if they report pass or fail, how do you convey that information through?

Comment 11

[Bob Clary [:bc] (inactive)]

(In reply to comment #10)

> > 1. ability to use 'fails' with 'load'. 
> 
> I don't see how this is useful without something else.

right. see 3.

> 
> > 2. ability to distinguish debug from non-debug builds.
> > 3. the ability to distinguish known from unexpected failures.
> 
> reftest already has this; it's what the "fails" keyword is for.

right, which is why I want to use 'fails' for 'load' tests.

> 
> 
> 
> A few questions about the JS tests:
>  * do the tests report pass or fail, or are they simply "not crash" tests?

they consist of both actually.

>  * if they report pass or fail, how do you convey that information through?

It partly depends on the subsuite, but the old-style jsDriver uses the output from reportFailure(). See http://mxr.mozilla.org/mozilla-central/source/js/tests/shell.js#188 which essentially writes " FAILED! " plus some description to the output for failed tests.

I kludged the existing framework to add an array of testcase results and use a dump function to produce a single line per result. see http://mxr.mozilla.org/mozilla-central/source/js/tests/shell.js#140

Rather than bothering you with making a change to reftest.js I was initially hoping to be able to communicate directly to the test whether it was expected to fail or not, so it could emit the appropriate REFTEST-sytle failure message. But that turned out to be more difficult than I thought. But since reftest.js already knows if a test should fail or not, Ted thought it would be nice if it could check the result of the test and emit the appropriate message itself.

Ted's idea was to just give reftest.js a single pass/fail indication perhaps through an exception but the reftest.js won't see content exceptions but it could look inside the test content to get a pass fail indicator. I thought if it could do that, it could easily output the results from an array of test results. Rather than force reftest.js to use the gTestcases array and its properties, I could easily make something available that would be more appropriate for general use.

Comment 12

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

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


r=dbaron on everything except for the following two pieces:

>             if (items.length != 2 ||
>                 (expected_status != EXPECTED_LOAD &&
>+                 expected_status != EXPECTED_FAIL &&
>                  expected_status != EXPECTED_DEATH))


>-    if (gURLs[0].expected == EXPECTED_LOAD) {
>+    if (gURLs[0].expected == EXPECTED_LOAD ||
>+        (gURLs[0].expected == EXPECTED_FAIL && !gURLs[0].url2)) {
>         ++gTestResults.LoadOnly;


These two pieces seem like they're part of something else, and I really don't understand yet what that is.

Comment 13

[Bob Clary [:bc] (inactive)]

Attachment: patch v2 without load changes

Ok. Here is a patch with only the isDebugBuild and manifest error message changes.

Comment 14

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/5730f4c1d016
http://hg.mozilla.org/tracemonkey/rev/9d5b7c57cf42

Comment 15

[Bob Clary [:bc] (inactive)]

This was landed on 1.9.2 on Oct 1, 2009
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1355b9de76eb