469718 - (jsreftests) Browser JavaScript Reftests

Status: RESOLVED FIXED

(Testing :: General, enhancement)

Description

[Bob Clary [:bc] (inactive)]

In order to better align the JavaScript tests with the other testing frameworks and to make them automatically run on unittest Tinderboxen, the JavaScript Test library should be modified to run when performing |make check| in the js/src directory.

Use js/src/Makefile.in to include make check like targets. See rules.mk for an example.

Use manifest files like reftests to list tests and their expected results

Use the same format as https://wiki.mozilla.org/Build:TinderboxErrors for the output so that Tinderbox can parse the results.

post process log, output pass summary but if the test fails, output the test log.

reuse jsDriver.pl and add an option to process the manifest list.

make sure jsDriver can handle getting the base directory passed to it, the manifest file will contain a list of tests to be run as paths relative to the base directory.

possible test result messages in:
http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js#507

jsDriver.pl would have to read the autoconf.mk variables. search for autoconf.js to find an example of converting the autoconf variables to a js script.

modify the manifest file to include a trailer portion of the line that could be used to specify environment variables (for example locale or language or timezone). jsDriver would set up the environment for the js shell and clean it up afterwards.

js/src/Makefile.in -> objdir/js/src/Makefile

objdir/js/src/Makefile would point back to srcdir/js/tests as the base test directory.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

We should make sure this works with Ted's latest changes to runreftests.py.

I know that Ted had made "make package-tests" combine all the reftest binaries, tests, and dependencies.

Another thing is this is ran with the runreftests.py now instead of the old method of <path>/firefox.exe -reftest <manifest>.

My personal involvement is that it will run (or be easily ignored) on Fennec.

Comment 2

[(not currently active) Ted Mielczarek]

This is mostly unrelated to actual reftest, except that we'd like to use a manifest similar to reftest.

Comment 3

[Bob Clary [:bc] (inactive)]

Attachment: patch - work in progress

This is a work in progress patch. 

I've changed the approach from what I outlined in the email to several of you. This approach is modeled more closely on crashtests. This supports running the JavaScript Tests in the browser using the reftest framework with either file: or http: urls using "load" in the same fashion as the crashtests. It requires bug 496846 in order to run the tests via a file url.

This includes a modification to the manifest parsing in layout/tools/reftest/reftest.js to allow load tests to be specified with fail. This should not affect existing crashtests as the failing crashtests in the tree are all specified with skip rather than load. If this is not acceptable, then it would be possible to mark failing tests with skip rather than load thus making any failure unexpected, however we would lose possible coverage of fatal js assertions for the failing tests.

The reftest framework automatically handles crashes and time outs. The intention is to use load tests which are told (somehow) if a test failure is expected or unexpected.
 
remaining work for the browser tests:

* Implement make targets in testing/testsuite-targets.mk

* The tests do not yet have a way to know if they should emit TEST-UNEXPECTED-FAIL or TEST-KNOWN-FAIL. Currently, the emit TEST-UNEXPECTED-FAIL for individual test failures.

* Update the manifests to skip additional slow tests which time out.

* Update the (fails|skip)-if(condition) in the manifests to only skip tests as appropriate... fatal assertions in debug builds, platform dependent failures, etc.

The manifests are named:

jstests.list            for the shell (not implemented yet).
jstestsbrowserfile.list for the browser using file: urls
jstestsbrowserhttp.list for the browser using http: urls.

To run the file: based browser tests:

python firefox-debug/_tests/reftest/runreftest.py --extra-profile-file=js/tests/user.js js/tests/jstestsbrowserfile.list

Note: this takes about 27-30 minutes on my macbook.

To run the http: based browser tests:

python firefox-debug/_tests/reftest/runreftest.py --extra-profile-file=js/tests/user.js js/tests/jstestsbrowserhttp.list

The extra profile file enables UniversalXPConnect for file:// or http://localhost:4444/

For the shell tests, I plan to fork testing/xpcshell/runxpcshelltests.py. I will also need a manifest parser with the ability to evaluate the ...-if(condition) and a way to communicate to the individual tests about expected failures.

Comment 5

[Bob Clary [:bc] (inactive)]

Attachment: patch v2

Ted, 

Most of this patch won't be interesting to you, but I'd appreciate your feedback on the changes to js/src/Makefile.in, js/tests/Makefile.in and testing/testsuite-targets.mk.

Rob, 

This will allow the developers to run the JavaScript Tests in the browser in a very similar fashion that the reftests are run:

make -C firefox-opt jstestbrowser

An example log is at http://bclary.com/log/2009/08/20/jstestbrowser.log.gzip

The next step will be to do this for the shell.

The known failures will be maintained as reftest-style manifest lists: jstests.list for the shell (not yet implemented), and jstestsbrowser.list for the browser.

David, 

I'm using the reftest extension, but am relying on it being built as part of the actual reftests. Is that ok?

Comment 6

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

(In reply to comment #5)
> I'm using the reftest extension, but am relying on it being built as part of
> the actual reftests. Is that ok?

I'm actually not really sure what you're asking.  But I suspect it may be a more appropriate question for ted than for me, anyway.  I don't know much about packaging.

Comment 7

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

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

In any case, I can't see anything here that I think needs my review, so cancelling the review request to me; review from ted and sayrer should be fine.

(See also previous comment.)

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: patch v3

This includes updates to the manifest files and a reworked packaging.

Comment 9

[Bob Clary [:bc] (inactive)]

Attachment: patch v4

now with linux 32/64bit goodness, and windows manifest updates. Ted, I removed some extraneous not ready shell test stuff. Please review this today so I can land on tracemonkey this evening.

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 399495 [details] [diff] [review]
patch v4

nit: don't use tabs in Makefiles for anything but the commands after rules. Use spaces everywhere else.

+TEST_FILES = \
...
+	js1_8_1/

We end lists like this with $(NULL), so that adding/removing items doesn't change blame excessively. (You can find plenty of examples in makefiles.)

+stage-package:
+	$(NSINSTALL) -D $(PKG_STAGE)/jsreftest && $(NSINSTALL) -D $(PKG_STAGE)/jsreftest/tests

You can just $(NSINSTALL) -D $(PKG_STAGE)/jsreftest/tests and it will create both directories in one go.

+	@(echo $(TEST_FILES)) \
+          | (cd $(srcdir) && xargs tar $(TAR_CREATE_FLAGS) -) \
+          | (cd $(PKG_STAGE)/jsreftest/tests && tar -xf -)

Write this like:
(cd $(srcdir) && xargs tar $(TAR_CREATE_FLAGS) - $(TEST_FILES)) | ...

Just like: http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/Makefile.in#78

+jstestbrowser: EXTRA_TEST_ARGS += --extra-profile-file=$(topsrcdir)/js/tests/user.js

This is sort of ugly, I hate target-specific variable overrides. Is there any reason we can't just make those prefs the defaults in reftest? I wouldn't block landing on this, I just dislike it.

+  jstestshell jstestbrowser \

Looks like you didn't actually add a 'jstestshell' target, just remove this for now?

Fix those things up and r=me.

Comment 11

[(not currently active) Ted Mielczarek]

Also, the title of this bug and the content of your patch don't match. Can you change the summary to more accurately reflect what you've implemented?

Comment 12

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/1870d316eb00

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Backed out since lw and I were getting build failures:

http://hg.mozilla.org/tracemonkey/rev/60e79c47c54a

Looks like maybe some file just got missed or something? This was on both OS X and Ubuntu.

Makefile:42: ../../config/autoconf.mk: No such file or directory
../config/config.mk:57: ../../config/autoconf.mk: No such file or directory
touch: ../../config/myrules.mk: No such file or directory
touch: ../../config/myconfig.mk: No such file or directory
make[2]: *** No rule to make target `../../config/autoconf.mk'.  Stop.
make[1]: *** [export] Error 2
make: *** [default] Error 2

Comment 14

[Bob Clary [:bc] (inactive)]

Firefox builds ok, but the shell is busted. :-( sorry. Thanks for backing this out.

Comment 15

[Bob Clary [:bc] (inactive)]

The problem is when the js/src/Debug/Makefile is created from js/src/Makefile.in 

The relative path in

ifdef ENABLE_TESTS
DIRS += ../tests
endif

ends up creating js/src/tests/Makefile which is not what is desired at all.

Comment 16

[Jesse Ruderman]

Please don't use separate manifests for shell and browser.  Use a single manifest with things like fails-if(isShell) and skip-if(isBrowser&&isDebugBuild).  Duplication in manifests is guaranteed to lead to pain and confusion; in fact, they're already out of sync in the checkin above.

Comment 17

[Bob Clary [:bc] (inactive)]

(In reply to comment #16)
> Please don't use separate manifests for shell and browser.  Use a single
> manifest with things like fails-if(isShell) and
> skip-if(isBrowser&&isDebugBuild).  Duplication in manifests is guaranteed to
> lead to pain and confusion; in fact, they're already out of sync in the checkin
> above.

I agree that it is an extra step for the developers and the files can easily get out of sync in terms of not containing the same files. It is also easy to check that the manifests contain all of the tests and that the two sets contain the same tests. I use a script to do that now. 

Which manifests are out of sync now? Apart from missing the recently added ecma_5/Object directory, I don't show any differences in the tests listed in the manifests.

They don't agree on the failure-types since I haven't tried to keep the shell manifests updated with respect to failure-types until I can run the shell tests using the manifests.

I would prefer to try to come up with a solution that didn't require changing the reftest extension. I doubt dbaron would want to add isShell to the reftest sandbox although isBrowser might be acceptable. 

If we agreed that the shell test runner will recognize xulRuntime.shell as true, then we could do what you want like fails-if(xulRuntime.shell) and skip-if(!xulRuntime.shell&&isDebugBuild). I think that would work fine with the exception of a reference to an undefined property warning in the browser.

If that is acceptable I'll use only the format for the browser manifests but keep them in manifest files called jstests.list and drop the dual manifest files altogether.

Comment 18

[Bob Clary [:bc] (inactive)]

Following a suggestion from Ted, I tested moving js/tests to js/src/tests and after minor changes was able to build opt/debug shell/firefox and to perform the packaging.

Brendan, Rob: Is it ok to move js/tests to js/src/tests ?

Comment 19

[Brendan Eich [:brendan]]

Don't js/tests still work in Rhino? I don't see why we need to push stuff down and make longer pathnames.

Of course, I want to boot js/src/xpconnect up to js/xpcom, e.g.

/be

Comment 20

[(not currently active) Ted Mielczarek]

I'd say just do js/src/tests as a quick switch to get it working, and then we can talk about moving all of js/src/ to js/*, and ditch xpconnect to elsewhere.

Comment 21

[Brendan Eich [:brendan]]

Ok, but do file that followup bug -- it should not be just idle talk. Rhino folks at least will notice and we haven't consulted them.

This is all cuz of autoconf etc., I take it.

/be

Comment 22

[Bob Clary [:bc] (inactive)]

Yes, I believe the autoconf build system for js is the motivation. Perhaps there is a better short term way of including js/tests into the build system, but moving the tests to js/src/tests works handily.

Norris, would (temporarily) moving js/tests to js/src/tests cause you undue hardship with respect to Rhino testing? 

Filed bug 516042 for reorganizing the js source directory.

Comment 23

[Bob Clary [:bc] (inactive)]

Attachment: patch v5

This version moves js/tests to js/src/tests.

It uses a single manifest file using the syntax for the browser reftests, .i.e. uses ../../jsreftest.html?test=... to load the script for the browser. The idea is to make dmandelin's jstest harness strip off the html reference when it loads the manifest.

If necessary we could make the manifest simply use the name of the test file however that would require a build step to produce the browser manifests in the OBJDIR. I prefer not to do that, but will leave it up to others.

Firefox (depends and clobber) and the js shell build on Mac OS X, Linux, Windows XP.

Comment 24

[Jesse Ruderman]

Why would just-filenames require an extra build step?  Can't reftest.js just be told where jsreftest.html lives and how to construct URLs using it when it sees a "script" line?

Comment 25

[Bob Clary [:bc] (inactive)]

The original idea was to leverage the reftest framework with minimal disruption/changes to its dna. The script keyword change to reftest.js was not "sold" as being a js only change, but one that could be used by anyone who had a similar need. An additional command argument might be one approach to special case the behavior. I don't think dbaron would be happy with a jstest only change like that, but I could be wrong. I'll let him state his position here if he care to do so.

If we fork the reftest extension altogether and make it js specific we are free to do whatever we want.

sayrer: your call.

Comment 26

[Bob Clary [:bc] (inactive)]

mmm, I guess sayrer is on vacation. hot potato for brendan!

Comment 27

[Bob Clary [:bc] (inactive)]

Attachment: patch v6

moves js/tests to js/src/tests
adds url-prefix to reftest manifests
uses single manifest files for shell and browser.

Comment 28

[(not currently active) Ted Mielczarek]

Comment on attachment 401738 [details] [diff] [review]
patch v6

r=me on the build bits, since you've got dbaron for review on the reftest changes.

Comment 29

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

Comment on attachment 401738 [details] [diff] [review]
patch v6

I'm not sure why you have so little context in your diff.  But aynawy...


>diff --git a/layout/tools/reftest/print-manifest-dirs.py b/layout/tools/reftest/print-manifest-dirs.py
>--- a/layout/tools/reftest/print-manifest-dirs.py
>+++ b/layout/tools/reftest/print-manifest-dirs.py
>@@ -71,3 +71,5 @@ def parseManifest(manifest, dirs):
> 
>-    if items[0] == "include":
>+    if items[0] == "url-prefix":
>+      continue
>+    elif items[0] == "include":
>       parseManifest(os.path.join(manifestdir, items[1]), dirs)

I think you ought to make print-manifest-dirs.py handle url-prefix rather than just ignoring it.  I think you can get it all in once place by modifying the "for u in testURLs" loop (assuming you make it not apply to include; see next comment).

>@@ -368,2 +374,11 @@ function ReadManifest(aURL)
> 
>+        if (urlprefix) {
>+            if (items.length > 1) {
>+                items[1] = urlprefix + items[1];
>+            }
>+            if (items.length > 2) {
>+                items[2] = urlprefix + items[2];
>+            }
>+        }
>+
>         if (items[0] == "include") {


So this actually applies the url prefix to 'include' commands as well.  I tend to think you probably don't want to.  Probably the easiest way to change this is to say if (urlprefix && items[0] != "include") at the top of this added chunk.  But either way I think README.txt should document whether or not url-prefix applies to include.

With those fixed, r=dbaron on the layout/reftest changes.

Comment 30

[Bob Clary [:bc] (inactive)]

(In reply to comment #29)
> (From update of attachment 401738 [details] [diff] [review])
> I'm not sure why you have so little context in your diff.  But aynawy...
> 

I'm just attaching the mq patch from .hg/patches/. Maybe that is not the right way to submit patches for review? Should I apply the patch and use hg qdiff to create the patch for review?

I've got 

[defaults]
diff=-U 8 -p
qdiff=-U 8 -p

[diff]
git=1
showfunc=1
unified=1

in my .hgrc fwiw.

> 
> >diff --git a/layout/tools/reftest/print-manifest-dirs.py b/layout/tools/reftest/print-manifest-dirs.py
> >--- a/layout/tools/reftest/print-manifest-dirs.py
> >+++ b/layout/tools/reftest/print-manifest-dirs.py
> >@@ -71,3 +71,5 @@ def parseManifest(manifest, dirs):
> > 
> >-    if items[0] == "include":
> >+    if items[0] == "url-prefix":
> >+      continue
> >+    elif items[0] == "include":
> >       parseManifest(os.path.join(manifestdir, items[1]), dirs)
> 
> I think you ought to make print-manifest-dirs.py handle url-prefix rather than
> just ignoring it.  I think you can get it all in once place by modifying the
> "for u in testURLs" loop (assuming you make it not apply to include; see next
> comment).
> 

I don't understand what you mean by "handle" the url-prefix here. Do you want the url-prefix to be prepended to the |u| url used to determine the directories to be added?

    for u in testURLs:
      if u.startswith("about:") or u.startswith("data:"):
        # can't very well package about: or data: URIs
        continue
      d = os.path.dirname(os.path.normpath(os.path.join(manifestdir, u)))
                                                                    ^^^
                                                      prepend url-prefix here?
      dirs.add(d)


I wouldn't think that the url-prefix should affect the directories or manifests loaded during print-manifest-dirs.py manifest parsing. I thought its use would only affect the url loaded by the reftest extension rather than determining the physical layout of the tests and manifests on the disk.

I just noticed that I don't distinguish data: or about: urls in reftest.js. I should not use the url-prefix for data: or about:, right? Are there other protocols I should not add the prefix to?


> >@@ -368,2 +374,11 @@ function ReadManifest(aURL)
[snip]...
> 
> So this actually applies the url prefix to 'include' commands as well.  I tend
> to think you probably don't want to.  Probably the easiest way to change this
> is to say if (urlprefix && items[0] != "include") at the top of this added
> chunk.  But either way I think README.txt should document whether or not
> url-prefix applies to include.

Agreed, that was not my intent.

Comment 31

[Graydon Hoare :graydon]

(In reply to comment #30)

> [diff]
> git=1
> showfunc=1
> unified=1

Change to "unified=8".

Comment 32

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

(In reply to comment #30)
> I've got 
> 
> [defaults]
> diff=-U 8 -p
> qdiff=-U 8 -p
> 
> [diff]
> git=1
> showfunc=1
> unified=1

Yep, change unified=1 to unified=8, and then you shouldn't need the diff or qdiff lines in the [defaults] section.  (Though I'd suggest qnew = -U there.)

> I don't understand what you mean by "handle" the url-prefix here. Do you want
> the url-prefix to be prepended to the |u| url used to determine the directories
> to be added?
> 
>     for u in testURLs:
>       if u.startswith("about:") or u.startswith("data:"):
>         # can't very well package about: or data: URIs
>         continue
>       d = os.path.dirname(os.path.normpath(os.path.join(manifestdir, u)))
>                                                                     ^^^
>                                                       prepend url-prefix here?
>       dirs.add(d)

Exactly.


> I wouldn't think that the url-prefix should affect the directories or manifests
> loaded during print-manifest-dirs.py manifest parsing. I thought its use would
> only affect the url loaded by the reftest extension rather than determining the
> physical layout of the tests and manifests on the disk.

If the url-prefix has a / in it, it can certainly affect what directory the files are in.

I think the point of print-manifest-dirs is to list the directories that contain files that need to be packaged.

> I just noticed that I don't distinguish data: or about: urls in reftest.js. I
> should not use the url-prefix for data: or about:, right? Are there other
> protocols I should not add the prefix to?

I'm tempted to say that we should either:
 * apply url-prefix to everything (which would make about:, data:, etc., not work), or
 * apply url-prefix to anything that doesn't have a protocol (i.e., looks like a relative URL)

Maybe the latter is better, if it's easy to do.  I suppose you could just check whether the string matches /^\w+:/.

Comment 33

[Bob Clary [:bc] (inactive)]

(In reply to comment #32)

    for u in testURLs:
      if u.startswith("about:") or u.startswith("data:"):
        # can't very well package about: or data: URIs
        continue
      d = os.path.dirname(os.path.normpath(os.path.join(manifestdir, u)))
      dirs.add(d)

> Maybe the latter is better, if it's easy to do.  I suppose you could just check
> whether the string matches /^\w+:/.

Should I go ahead and change print-manifest-dirs.py to also use the same pattern to skip protocol-like uris instead of the hardwired about: or data: above ?

Comment 34

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

(In reply to comment #33)
> Should I go ahead and change print-manifest-dirs.py to also use the same
> pattern to skip protocol-like uris instead of the hardwired about: or data:
> above ?

Yes.

Comment 35

[Bob Clary [:bc] (inactive)]

Attachment: patch v7

David, 

Do you wish to re-review this? I believe I have addressed all of the issues. I have also added some reftest-sanity tests for script and for url-prefix.

I tested reftest, crashtest, jstestbrowser, and package-tests locally. I also did a try server build which passed for reftest and crash test.

Comment 36

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

Comment on attachment 402975 [details] [diff] [review]
patch v7

+        if (items[0] == "url-prefix") {

Actually, right here you should insert a check to throw an exception if items.length != 2.  (Sorry for missing this earlier.)

+            urlprefix = items[1];
+            continue;
+        }


Also, maybe put /^\w+:/ in a global variable with a useful name, like you did in the python one?

r=dbaron with that fixed; no need to ask again

Comment 37

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/7b0823541920

Lets hope this sticks. Thanks everyone.

Comment 38

[Igor Bukanov]

Attachment: fixing document reference

The patch protects the document reference in the dump function with the typeof document check so the tests can still run using jsDriver.

Comment 39

[Bob Clary [:bc] (inactive)]

Comment on attachment 403219 [details] [diff] [review]
fixing document reference

Doh. Thanks Igor.

Comment 40

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/fb92e8e990ce - landed followup

Comment 41

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/7b0823541920

Comment 42

[Robert Sayre]

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

Comment 43

[Nickolay_Ponomarev]

dev-doc-needed: this (and information from http://groups.google.com/group/mozilla.dev.tech.js-engine/browse_frm/thread/36cdda5bf703e544/28c10d32b4517652?tvc=1#28c10d32b4517652 ) should probably be mentioned at https://developer.mozilla.org/en/Mozilla_automated_testing