Created attachment 342141 [details] [review]
coverage test: Fix copypasta errors

A few coverage tests didn't test what they said the tested. One was
identical to a test a few lines above. The other had a trailing newline
whereas the description said it was testing the case without a trailing
newline.

Created attachment 342142 [details] [review]
coverage test: Fix equality error

There was an error in the functionDeclarationsEqual() tester which caused
some tests not to fail that should have failed. I believe these are
errors in the expected values in the tests, rather than actual errors in
the code.

Created attachment 342143 [details] [review]
test everything: Enable int64 array test

I implemented support for this, so the test can be uncommented.

Created attachment 342144 [details] [review]
tests: Move logError test to testExceptions.js

This didn't really test stuff from the Regress library, so it should be
moved to the testExceptions file. As well, the test never asserted that
the expected messages were logged, so it would not have caught any test
failures.

Created attachment 342145 [details] [review]
tests: Move GVariant overrides test to testGLib.js

This didn't really belong in testEverythingEncapsulated, which tests
boxed types from the Regress library. Instead we put it in a new file
which will hold any other tests of GLib overrides added in the future.

Created attachment 342146 [details] [review]
tests: Move GType tests to testGTypeClass.js

These didn't belong in testGIMarshalling, since they are testing the
native GType object and/or the GObject overrides for GType.

Created attachment 342147 [details] [review]
test marshalling: Disable foreign struct test

This test fails when running as an installed test. It seems that JSUnit
swallows the exception somehow during "make check".

Created attachment 342148 [details] [review]
test locale: Delete commented-out test

This was supposed to have been commented out temporarily, but I'm not
aware of a way in which to create an invalid JSString that can't be
converted to UTF-8, at least not from Javascript. Probably the '\ud800'
triggered a bug in an earlier version of SpiderMonkey, but I don't think
this test can sensibly pass anymore.

Created attachment 342149 [details] [review]
tests: Consolidate importer tests

test0020importer.js can be moved into testImporter.js, so that all
importer tests are in the same place.

Created attachment 342150 [details] [review]
tests: Delete redundant tests

- 0010basic: This only tests 1 + 1 = 2. The internal consistency of the
  test harness is already tested more thoroughly in testself.js.
- 0030basicBoxed: This duplicates functionality which was tested more
  thoroughly in testEverythingEncapsulated.js.
- 0040mainloop: This duplicates functionality which was tested more
  thoroughly in testMainloop.js.
- JS1_8: This tests SpiderMonkey features, and has nothing to do with
  any code in GJS.
- ReflectObject: This also tests SpiderMonkey features.
- Unicode: This tests SpiderMonkey's ability to handle Unicode
  characters, also nothing to do with GJS.

Created attachment 342151 [details] [review]
tests: Use embedded copy of Jasmine to run tests

This replaces the old JSUnit test harness with an embedded copy of
Jasmine [1], which makes writing tests less of a pain, includes more
handy test facilities, and produces better output.

jasmine.js is a copy of upstream Jasmine 2.5.2. minijasmine.js contains
code for starting up Jasmine, adapting it to the GJS environment, and
producing TAP output. minijasmine.cpp makes an executable which loads the
preceding two files from the unit test GResource.

All the tests in installed-tests/js are converted to use Jasmine's
describe()/it() style. Quite often this allows simplifying them since
Jasmine has features like array and object equality, spies, and clock
ticks.

[1] https://jasmine.github.io/2.5/introduction.html

Created attachment 342152 [details] [review]
tests: Run testCommandLine.sh installed

This file can be run just as well as an installed test, so we install it
to the correct directory and add a .test file.

The script now checks for GJS_USE_UNINSTALLED_FILES and tests the
uninstalled copy of gjs-console if it is set to 1.

Review of attachment 342141 [details] [review]:

OK

Review of attachment 342142 [details] [review]:

OK

Review of attachment 342143 [details] [review]:

OK

Review of attachment 342144 [details] [review]:

Good catch

Review of attachment 342145 [details] [review]:

Sure

Review of attachment 342146 [details] [review]:

Sure

Review of attachment 342147 [details] [review]:

Maybe leave a comment? Does this go away when we port to Jasmine?

Review of attachment 342148 [details] [review]:

OK

Review of attachment 342149 [details] [review]:

Sure

Review of attachment 342150 [details] [review]:

OK

Review of attachment 342151 [details] [review]:

I did not look at all the tests in details, but I trust that they all pass after this :)

::: installed-tests/js/testClass.js
@@ +184,2 @@
+            _construct: function(one, two) {
+                return [one, two];

I did not know this worked!

::: installed-tests/minijasmine.cpp
@@ +60,3 @@
+    setlocale(LC_ALL, "");
+
+    if (g_getenv ("GJS_USE_UNINSTALLED_FILES") != NULL) {

Extra space before paren

Review of attachment 342152 [details] [review]:

Sure

(In reply to Cosimo Cecchi from comment #19)
> Review of attachment 342147 [details] [review] [review]:
> 
> Maybe leave a comment? Does this go away when we port to Jasmine?

The test still fails under Jasmine, indicating a bug in GJS. In the Jasmine commit I'm skipping it with xit() and a custom message explaining why, so I don't think it's necessary to add a comment here. This commit was just so that it didn't look like I was taking a passing test and replacing it with a skipped test.

(In reply to Cosimo Cecchi from comment #23)
> Review of attachment 342151 [details] [review] [review]:
> ::: installed-tests/js/testClass.js
> @@ +184,2 @@
> +            _construct: function(one, two) {
> +                return [one, two];
> 
> I did not know this worked!

Do you mean returning a custom object from a constructor? It's one of the bizarre quirks of JS and probably not such a great idea to use since it's pretty obscure.

Here's a stack overflow post explaining it in some more detail:
http://stackoverflow.com/a/3350364/172999

But this is one reason why I like Jasmine: it makes it easier to read casually what is the desired behaviour of your code!

I'll go ahead and push everything with the comments fixed.

Attachment 342141 [details] pushed as 2dec333 - coverage test: Fix copypasta errors
Attachment 342142 [details] pushed as 18c99a8 - coverage test: Fix equality error
Attachment 342143 [details] pushed as 8eb23fa - test everything: Enable int64 array test
Attachment 342144 [details] pushed as cb45cf3 - tests: Move logError test to testExceptions.js
Attachment 342145 [details] pushed as aa267dc - tests: Move GVariant overrides test to testGLib.js
Attachment 342146 [details] pushed as 4c93f88 - tests: Move GType tests to testGTypeClass.js
Attachment 342147 [details] pushed as 7ae93af - test marshalling: Disable foreign struct test
Attachment 342148 [details] pushed as 8484748 - test locale: Delete commented-out test
Attachment 342149 [details] pushed as 96dfa29 - tests: Consolidate importer tests
Attachment 342150 [details] pushed as bf11496 - tests: Delete redundant tests
Attachment 342151 [details] pushed as 3d6e9fa - tests: Use embedded copy of Jasmine to run tests
Attachment 342152 [details] pushed as 413bd40 - tests: Run testCommandLine.sh installed