Created attachment 344241 [details] [review]
A random first step

Review of attachment 344241 [details] [review]:

Thanks for the patch! It's a good start.

::: installed-tests/js/testSystem.js
@@ +15,3 @@
 describe('System.version', function () {
     it('gives a plausible number', function () {
+        expect(System.version).not.toBeLessThan(14700);

I'm fine to update this for now, but I don't think it's something that needs to be kept up to date with every release unless it's hooked up to happen automatically based on the output of configure.

As a middle ground maybe something like this?

    expect(System.version).not.toBeLessThan(14700);
    expect(System.version).toBeLessThan(20000);

...at least until there is a 2.0.0...

@@ +28,3 @@
+describe('System.gc()', function () {
+    it('works properly', function () {
+        expect(System.gc()).not.toBeDefined();

Like I said in my email it's nearly impossible to verify "works properly" from inside Javascript code.

I could suggest two approaches here:
(1) rename the test "it does not crash the application" and just System.gc(), nothing else. (easy)
(2) write a test in C that actually sets up an unrooted JSObject and a weak pointer to it, then calls gjs_context_gc() or evaluates the string "imports.system.gc()", and makes sure the weak pointer is set to NULL. (difficult, maybe not worth the effort.)

@@ +34,3 @@
+describe('System.clearDateCaches()', function () {
+    it('works properly', function () {
+        expect(System.clearDateCaches()).not.toBeDefined();

For this one I'd be interested in knowing what exactly clearDateCaches() is supposed to do :-)

(In reply to Philip Chimento from comment #2)
> Review of attachment 344241 [details] [review] [review]:
> 

So, let's avoid the over engineering.

> @@ +34,3 @@
> +describe('System.clearDateCaches()', function () {
> +    it('works properly', function () {
> +        expect(System.clearDateCaches()).not.toBeDefined();
> 
> For this one I'd be interested in knowing what exactly clearDateCaches() is
> supposed to do :-)

To test this properly (and document exactly what clearDateCaches is doing), I need to be able to insert a shell command in the test.

    let date = new Date('12/15/1981 23:00');
    run('ln -sf /usr/share/zoneinfo/Europe/Amsterdam /etc/localtime');
    System.clearDateCaches();

    //Check if locale was "refreshed" doing something clever
    let datestr = date.toLocaleDateString('something clever');

Created attachment 345051 [details] [review]
A random first step (1)

Review of attachment 345051 [details] [review]:

+1, if we leave out the clearDateCaches() test for now.

Sorry for the delay in reviewing.

::: installed-tests/js/testSystem.js
@@ +29,3 @@
+describe('System.gc()', function () {
+    it('does not crash the application', function () {
+        expect(System.gc()).not.toBeDefined();

Let's change this to expect(() => System.gc()).not.toThrow(); as a more "neutral" expectation.

@@ +33,3 @@
+});
+
+describe('System.clearDateCaches()', function () {

I'll answer your other question about this one shortly. But let's leave it out for now.

(In reply to Claudio André from comment #3)
> To test this properly (and document exactly what clearDateCaches is doing),
> I need to be able to insert a shell command in the test.
> 
>     let date = new Date('12/15/1981 23:00');
>     run('ln -sf /usr/share/zoneinfo/Europe/Amsterdam /etc/localtime');
>     System.clearDateCaches();
> 
>     //Check if locale was "refreshed" doing something clever
>     let datestr = date.toLocaleDateString('something clever');

Hmm, difficult. That would potentially require the tests to have root access. Would it work to use the TZ environment variable instead? [1] That seems to override the on-disk value.

[1] http://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html

(In reply to Philip Chimento from comment #6)
> >     let date = new Date('12/15/1981 23:00');
> >     run('ln -sf /usr/share/zoneinfo/Europe/Amsterdam /etc/localtime');
> >     System.clearDateCaches();
> > 
> >     //Check if locale was "refreshed" doing something clever
> >     let datestr = date.toLocaleDateString('something clever');
> 
> Hmm, difficult. That would potentially require the tests to have root
> access. Would it work to use the TZ environment variable instead? [1] That
> seems to override the on-disk value.
> 
> [1] http://www.gnu.org/software/libc/manual/html_node/TZ-Variable.html

In fact, I was expecting you are going to refuse the whole idea of changing the user environment. But setting TZ might be acceptable.

(In reply to Philip Chimento from comment #5)
> ::: installed-tests/js/testSystem.js
> @@ +29,3 @@
> +describe('System.gc()', function () {
> +    it('does not crash the application', function () {
> +        expect(System.gc()).not.toBeDefined();
> 
> Let's change this to expect(() => System.gc()).not.toThrow(); as a more
> "neutral" expectation.

It fails [toThrow() and not.toThrow()].

[1] https://travis-ci.org/claudioandre/gjs/builds/200389896

----
FAIL: installed-tests/js/testSystem.js 4 System.gc() does not crash the application
ERROR: installed-tests/js/testSystem.js - exited with status 1
----

(In reply to Claudio André from comment #8)
> (In reply to Philip Chimento from comment #5)
> > ::: installed-tests/js/testSystem.js
> > @@ +29,3 @@
> > +describe('System.gc()', function () {
> > +    it('does not crash the application', function () {
> > +        expect(System.gc()).not.toBeDefined();
> > 
> > Let's change this to expect(() => System.gc()).not.toThrow(); as a more
> > "neutral" expectation.
> 
> It fails [toThrow() and not.toThrow()].

I don't know whether this is the case, but I suspect you might be missing the `() =>`. In this case you have to pass a function to expect(), so either of the following two would work:

    expect(() => System.gc()).not.toThrow();
    expect(System.gc).not.toThrow();

Created attachment 345582 [details] [review]
A random first step

Passing all tests

Review of attachment 345582 [details] [review]:

+1

Hi Claudio, are you interested in picking this up again? We have a 1.49.1 release coming up in a week, in case you're interested in landing some more test coverage improvements before then.

One place I might suggest to start, is to look at regress.c and gimarshallingtests.c, and check which tests we _don't_ execute in our test suite, and why not. (Sometimes there might be a reason - in which case it might be better to write a test but skip it with xit() - and sometimes there might be no reason.)

At this moment, the poor test coverage is restricted to one area/lib. Well, when I tried to do something, I realized I was pretending to improve the tests; I mean, if I don't have enough knowledge of lib X, all the tests I can create aren't effective.

So, I'm afraid I'm not the right person to create the needed tests.

OK, I'll close this bug for now, if we don't have a good idea of what to do next. But I encourage you to reopen it if you want to continue investigating / learning about this.