464898 - Rhino standard tests don't run cleanly

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Marc]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.3) Gecko/2008092510 Ubuntu/8.04 (hardy) Firefox/3.0.3
Build Identifier: 

Running the tests with ant-junit all reports a lot of failed tests. Running tests as explained at https://developer.mozilla.org/en/Running_the_Rhino_tests doesn't report any failure (at least for interpreted mode).
This shows that there is a big problem at least in one of the two methods.

The first failing test reported when running the tests as junit tests is lc3/ArrayMethods/object-001.js. Which is not reported as failing by JsDriver.

If I add following as first statement of JsDriver.excluded:
            if (!path.contains("lc3/ArrayMethods/object-001.js"))
            	return true;
and run JsDriver tests again, then only this single test is run... and if fails exactly like when running it as unit test.

For me this shows a really big problem in the tests. Particularly this seems to mean that we can't trust the results of JsDriver and that the unit test result are (more?) correct. In which case, a lot of tests fail currently!

Additionally, do we really need JsDriver? It is important to be able to build and run the tests in automatic build process and have usable results that we can trust. JUnit seems better suited for this purpose.

I set the severity of this issue as critical because I think that we can't have any reliable information on Rhino's quality as long as it is not fixed.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Marc]

Attachment: Patch adding jstests.jar to the classpath for the unit tests

One part of the problem is that the junit test don't have jstests.jar in the classpath which contains com.netscape.*

On my computer this reduces the number of failed standard tests from 657 to 380.

Comment 2

[Hannes Wallnoefer]

Thanks Marc. I committed your patch to both HEAD and Rhino1_7R2_BRANCH.

Comment 3

[Marc]

Cool!

http://cn-rhino01.nl.mozilla.org:8085/browse/RHINO-HEAD-163 shows that if fixed 146 tests ... but that 241 are still failing :-(

Comment 4

[Hannes Wallnoefer]

Looking at the remaining failing bugs, it seems like the two biggest groups are failing due to the missing global options() function in Rhino, and because trying to call js1_8 tests and calling version(180). 

option() seems to be used to enable/disable the jit compiler - option() without arguments returns a string with current options status, and it looks like option('jit') should toggle the jit. I couldn't reproduce this in spidermonkey shell - options() always returns an empty string. Maybe spidermonkey always runs with jit and options() is left in for backwards compatibility?

The js 1.8 issue is strange, because the js1_8 directory is in the skip list. I guess something there's a problem with processing the skip lists here. 

BTW, in my local copy with the js 1.8 patches we have already applied, 29 of the js1_8 test cases pass already, which is nice :-)

Comment 5

[Hannes Wallnoefer]

Doh! StandardTests checks the skip list using Properties.containsKey(), but the skip list also contains directories (i.e. 'js1_8' for all tests in that directory). Patch coming soon.

Comment 6

[Hannes Wallnoefer]

Attachment: check skiplist for partial paths

I'm down to 24 failures with this patch. Still have to check out if there are more issues left, or if this was actually the only cause for the diverging results.

Comment 7

[Marc]

Isn't it possible to share the logic of test selection with JsDriver to avoid different results?

Comment 8

[Hannes Wallnoefer]

(In reply to comment #7)
> Isn't it possible to share the logic of test selection with JsDriver to avoid
> different results?

I guess it would be, and I guess that's the way it should be. JSDriver currently uses a String array and loops over it using startsWith() while StandardTests only checks on a directory/file level. Thus a skip list entry of "js1_8" covers directory "js1_8_1" in JSDriver, but does not in StandardTests. 

Any opinions on which behaviour is more desirable/compatible/whatever?

Comment 9

[Marc]

(In reply to comment #8)
> 
> Any opinions on which behaviour is more desirable/compatible/whatever?

I don't care personally. What matters is to be sure that the same logic is used.

Comment 10

[Hannes Wallnoefer]

Attachment: patch to unify exclude list handling in test drivers

Comment 11

[Hannes Wallnoefer]

Attachment: patch with added options() function

Most remaining bugs fail because of the missing options() function in spidermonkey. This patch extends the previous one to add a dummy options() function to the test scope.

Comment 12

[Hannes Wallnoefer]

Committed to HEAD.

Removing testsrc/org/mozilla/javascript/drivers/FileUtils.java;
/cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/drivers/FileUtils.java,v  <--  FileUtils.java
new revision: delete; previous revision: 1.2
done
Checking in testsrc/org/mozilla/javascript/drivers/JsDriver.java;
/cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/drivers/JsDriver.java,v  <--  JsDriver.java
new revision: 1.9; previous revision: 1.8
done
Checking in testsrc/org/mozilla/javascript/drivers/ShellTest.java;
/cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/drivers/ShellTest.java,v  <--  ShellTest.java
new revision: 1.11; previous revision: 1.10
done
Checking in testsrc/org/mozilla/javascript/drivers/StandardTests.java;
/cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/drivers/StandardTests.java,v  <--  StandardTests.java
new revision: 1.11; previous revision: 1.10
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/drivers/TestUtils.java,v
done
Checking in testsrc/org/mozilla/javascript/drivers/TestUtils.java;
/cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/drivers/TestUtils.java,v  <--  TestUtils.java
initial revision: 1.1
done
Checking in testsrc/org/mozilla/javascript/tests/DoctestsTest.java;
/cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/DoctestsTest.java,v  <--  DoctestsTest.java
new revision: 1.4; previous revision: 1.3
done
Checking in testsrc/org/mozilla/javascript/tests/JsTestsTest.java;
/cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/JsTestsTest.java,v  <--  JsTestsTest.java
new revision: 1.3; previous revision: 1.2
done

With this, I have 10 tests failing with jsdriver and 11 with ant/junit. The one differing test fails with a stack overflow which is likely due to the different testing environment. Marking bug as fixed.

Comment 13

[Marc]

You're the hero!

Does this mean that we will soon have a passed build? Shouldn't the failing tests be added to the skip list(s)?

Comment 14

[Hannes Wallnoefer]

Thanks :-) I think we should look at the failing tests one by one and decide in each case if it applies to Rhino. But yes, I think passing the test suite is now well within reach.

Comment 15

[Hannes Wallnoefer]

Committed last patch to Rhino1_7R2_BRANCH.