Created attachment 364551 [details] [review]
gtester: do not consider skipped tests as failures

With the patch:

$ gtester --verbose -s /webkit2/WebKitCookieManager/persistent-storage bin/TestWebKitAPI/WebKit2Gtk/TestCookieManager
TEST: bin/TestWebKitAPI/WebKit2Gtk/TestCookieManager... (pid=25587)
  /webkit2/WebKitCookieManager/accept-policy:                          OK
  /webkit2/WebKitCookieManager/add-cookie:                             OK
  /webkit2/WebKitCookieManager/get-cookies:                            OK
  /webkit2/WebKitCookieManager/delete-cookie:                          OK
  /webkit2/WebKitCookieManager/delete-cookies:                         OK
  /webkit2/WebKitCookieManager/cookies-changed:                        OK
  /webkit2/WebKitCookieManager/persistent-storage:                     SKIP
  /webkit2/WebKitCookieManager/persistent-storage-delete-all:          OK
  /webkit2/WebKitCookieManager/ephemeral:                              OK
PASS: bin/TestWebKitAPI/WebKit2Gtk/TestCookieManager

Created attachment 364553 [details] [review]
gtester: Accept 'skip' exit code as success

When running outside of a TAP harness, GTest uses exit code 77 for
skipped units, and considers whole skipped units to be successful
for the purposes of the test suite.

The gtester harness does not recognise this as a valid exit code, and
thus considers skipped units as failures.

My strong suggestion is for you to stop using gtester, and instead just use whatever test harness is available for CMake. gtester is not really maintained any more. The documentation has been updated lately to mention this fact.

Having said that, we should probably fix it.

Review of attachment 364551 [details] [review]:

Definitely better patch than mine, but you will still need to handle exit code 77, which currently is considered a failure case.

::: glib/gtester.c
@@ +114,3 @@
   test_log_printfe ("%s</testcase>\n", sindent (log_indent));
   testcase_open--;
+  if (gtester_verbose) {

Coding style of this whole block does not fit with GLib's own:

 - braces go on a separate indentation level
 - case blocks go on a separate indentation level

::: glib/gtestutils.h
@@ +360,3 @@
+  G_TEST_RUN_FAILURE,
+  G_TEST_RUN_INCOMPLETE
+} GTestResult;

Not a big fan, but okay…

(In reply to Emmanuele Bassi (:ebassi) from comment #4)
> My strong suggestion is for you to stop using gtester, and instead just use
> whatever test harness is available for CMake. gtester is not really
> maintained any more. The documentation has been updated lately to mention
> this fact.

We don't use CMake either, we have our script that is run by our bots. I can change the script to use whatever is recommended now, though.

> Having said that, we should probably fix it.

Indeed.

(In reply to Emmanuele Bassi (:ebassi) from comment #5)
> Review of attachment 364551 [details] [review] [review]:
> 
> Definitely better patch than mine, but you will still need to handle exit
> code 77, which currently is considered a failure case.

But 77 is returned when all the tests run were skipped, no? Not by individual tests.

> ::: glib/gtester.c
> @@ +114,3 @@
>    test_log_printfe ("%s</testcase>\n", sindent (log_indent));
>    testcase_open--;
> +  if (gtester_verbose) {
> 
> Coding style of this whole block does not fit with GLib's own:
> 
>  - braces go on a separate indentation level
>  - case blocks go on a separate indentation level

Ah, right, I'll fix it.

> ::: glib/gtestutils.h
> @@ +360,3 @@
> +  G_TEST_RUN_FAILURE,
> +  G_TEST_RUN_INCOMPLETE
> +} GTestResult;
> 
> Not a big fan, but okay…

Me neither, at first I copied the enum with a comment saying it should be kept in sync with gtestutils, but then I realized that other internal log API was exposed and used by gtester. So, I don't mind copying the enum if you prefer it.

Created attachment 364554 [details] [review]
gtester: do not consider skipped tests as failures

Fix coding style

(In reply to Carlos Garcia Campos from comment #6)
> (In reply to Emmanuele Bassi (:ebassi) from comment #4)
> > My strong suggestion is for you to stop using gtester, and instead just use
> > whatever test harness is available for CMake. gtester is not really
> > maintained any more. The documentation has been updated lately to mention
> > this fact.
> 
> We don't use CMake either, we have our script that is run by our bots. I can
> change the script to use whatever is recommended now, though.

I'd strongly encourage you to switch to a TAP harness; there are many ready to be used.

(In reply to Carlos Garcia Campos from comment #7)
> (In reply to Emmanuele Bassi (:ebassi) from comment #5)
> > Review of attachment 364551 [details] [review] [review] [review]:
> > 
> > Definitely better patch than mine, but you will still need to handle exit
> > code 77, which currently is considered a failure case.
> 
> But 77 is returned when all the tests run were skipped, no? Not by
> individual tests.

Yes, but gtester will still consider a test a success if exit_code == 0. It does not impact your precise issue, so it's not really a blocker, but it would be nice to do the right thing.

> > ::: glib/gtester.c
> > @@ +114,3 @@
> >    test_log_printfe ("%s</testcase>\n", sindent (log_indent));
> >    testcase_open--;
> > +  if (gtester_verbose) {
> > 
> > Coding style of this whole block does not fit with GLib's own:
> > 
> >  - braces go on a separate indentation level
> >  - case blocks go on a separate indentation level
> 
> Ah, right, I'll fix it.

Thanks.

> > ::: glib/gtestutils.h
> > @@ +360,3 @@
> > +  G_TEST_RUN_FAILURE,
> > +  G_TEST_RUN_INCOMPLETE
> > +} GTestResult;
> > 
> > Not a big fan, but okay…
> 
> Me neither, at first I copied the enum with a comment saying it should be
> kept in sync with gtestutils, but then I realized that other internal log
> API was exposed and used by gtester. So, I don't mind copying the enum if
> you prefer it.

No, let's move it. The gtestutils.h header is a mess anyway, it's not going to get any better any time soon.

Review of attachment 364554 [details] [review]:

Looks good to me.

::: glib/gtester.c
@@ +110,3 @@
   test_log_printfe ("%s<status exit-status=\"%d\" n-forks=\"%d\" result=\"%s\"/>\n",
                     sindent (log_indent), exit_status, n_forks,
+                    success ? "failed" : "success");

Sorry, I didn't catch this. Shouldn't the XML also report that the test was skipped? Of course, then gtester-report would need to deal with it. Meh, let's just ignore this.

Comment on attachment 364554 [details] [review]
gtester: do not consider skipped tests as failures

Pushed as https://git.gnome.org/browse/glib/commit/?id=ed620183cbb762eec8a0d0ff1575e5dc3acc329a

This change introduced a subtle bug in the XML output. Now all successful tests are reported as "failed", and all failed tests as "success".

::: glib/gtester.c
@@ +110,3 @@
   test_log_printfe ("%s<status exit-status=\"%d\" n-forks=\"%d\" result=\"%s\"/>\n",
                     sindent (log_indent), exit_status, n_forks,
+                    success ? "failed" : "success");

This should have been the other way around.

Created attachment 370885 [details] [review]
gtester: fix test result in gtester XML report

Emmanuele?

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1305.