505798 - JSAPI test harness

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

I've had this lying around gathering dust for a few months, and I think it's probably time to check it in.

Unfortunately I used C++ exceptions, but I think I can change those to assertions without too much fuss.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1

Uses exceptions, thus not built by default.

To build this, you need to apply the patch in bug 505522 and configure with --enable-cpp-exceptions.  Once you do that, `make` in your js objdir will automatically make jsapi-tests.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: v2

No longer uses exceptions at all. Builds by default. Responds to `make check`.

Comment 3

[Jason Orendorff [:jorendorff]]

Oh-- this actually contains two tests that don't pass at the moment. One of them is named 'addProperty' and should be fixed in bug 497789. The other tests for bug 506491, which should be fixed today.

I don't want to check in failing tests, so I'll comment them out for the initial check-in, if this gets r+ before those bugs are fixed.

Comment 4

[Jason Orendorff [:jorendorff]]

Found it: the 'addProperty' test hits bug 505523.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: v3

Changes from v2:

* I renamed the tests and the test filenames.

* I removed the trivial test 'jsfun'.

* I changed how the test objects register themselves. Before, I was using a std::map. This didn't work on Linux, I guess because there's no guarantee that static initializers in separate source files will run in any particular order. I could have fixed it by making things slightly more complicated; instead I simplified.

Comment 6

[(not currently active) Ted Mielczarek]

Drive by comments:
 ifndef JS_DISABLE_SHELL
 DIRS += shell
+DIRS += jsapi-tests
 endif

Any reason you stuck your dir inside this !JS_DISABLE_SHELL block? Also, if you're going to keep it there, no need for two +=, just make that line "DIRS += shell jsapi-tests".

+CPPSRCS		= tests.cpp \
+                  testPropCache.cpp \
+                  testXDR.cpp

You've got some literal tabs in there (after CPPSRCS). Also we tend to prefer:
CPPSRCS = \
  test.cpp \
  testPropCache.cpp \
  testXDR.cpp \
  $(NULL)

To future-proof against mucking up blame too much.

+check::
+	$(wildcard $(RUN_TEST_PROGRAM)) ./jsapi-tests$(BIN_SUFFIX)

I think you'll want to run from $(DIST)/bin, graydon had problems without that on win32, see bug 473989 comment 22.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 395403 [details] [diff] [review]
v3

>diff --git a/js/src/Makefile.in b/js/src/Makefile.in
>--- a/js/src/Makefile.in
>+++ b/js/src/Makefile.in
>@@ -52,16 +52,17 @@ endif
> 
> ifdef JS_NATIVE_EDITLINE
> DIRS += editline
> endif
> 
> # editline needs to get built before the shell
> ifndef JS_DISABLE_SHELL
> DIRS += shell
>+DIRS += jsapi-tests
> endif

Agree with ted, I think we should just always build the tests.  (I'm not much a fan of JS_DISABLE_SHELL either, truth be told.)


>diff --git a/js/src/jsapi-tests/tests.h b/js/src/jsapi-tests/tests.h

>+    explicit jsvalRoot(JSContext *context, jsval value = JSVAL_NULL)
>+        : cx(context), v(value)
>+    {
>+        JS_AddRoot(cx, &v);
>+    }

JS_AddRoot is fallible; please write to null if it fails or something, if exceptions are off-limits.


>+    bool eq(JSString *a, JSString *b) {
>+        return JS_CompareStrings(a, b) == 0;
>+    }
>+
>+    bool eq(const jsvalRoot &a, const jsvalRoot &b) {
>+        return bool(JS_StrictlyEqual(cx, a, b));
>+    }

Let me voice dissent at eq and support for "same", whose semantics would be those of the SameValue method in ES5.  Thoughts?  I would also prefer only having the one comparison routine, personally, not two that the test author can confuse, so I'd rather not see both.

Comment 8

[Jason Orendorff [:jorendorff]]

(In reply to comment #7)
> > ifndef JS_DISABLE_SHELL
> > DIRS += shell
> >+DIRS += jsapi-tests
> > endif
> 
> Agree with ted, I think we should just always build the tests.  (I'm not much a
> fan of JS_DISABLE_SHELL either, truth be told.)

I put it in an ifdef ENABLE_TESTS, which ted assures me is on by default.

> >+    explicit jsvalRoot(JSContext *context, jsval value = JSVAL_NULL)
> >+        : cx(context), v(value)
> >+    {
> >+        JS_AddRoot(cx, &v);
> >+    }
> 
> JS_AddRoot is fallible; please write to null if it fails or something, if
> exceptions are off-limits.

Done. I'm calling abort() if it fails. Hope that's ok.

> >+    bool eq(JSString *a, JSString *b) {
> >+        return JS_CompareStrings(a, b) == 0;
> >+    }
> >+
> >+    bool eq(const jsvalRoot &a, const jsvalRoot &b) {
> >+        return bool(JS_StrictlyEqual(cx, a, b));
> >+    }
> 
> Let me voice dissent at eq and support for "same", whose semantics would be
> those of the SameValue method in ES5.  Thoughts?  I would also prefer only
> having the one comparison routine, personally, not two that the test author can
> confuse, so I'd rather not see both.

OK. I did away with eq() entirely. Now there's just CHECK(bool) and
CHECK_SAME(jsval, jsval). I added two tests to check my implementation of CHECK_SAME; see selfTest.cpp. Maybe I should have gone ahead and poked a hole in the JSAPI for JS_SameValue(cx, v1, v2). Later.

I also added a field, knownFail, because one of the tests does currently fail, and I wanted to spit out TEST-KNOWN-FAIL for that one.

And I changed main() to return 1 if any tests unexpectedly fail.

I think that's pretty much it.

http://hg.mozilla.org/tracemonkey/rev/85cd77fad8b5

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/85cd77fad8b5

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

this seems to have broken windows ce

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

here is the build output http://pastebin.mozilla.org/668523

Comment 12

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/467dbdb8390f