Created attachment 238679 [details] [review]
Use gtester to run tests

Tristan, what do you think about this, please? It's related to your changes in output of tests, and it seems like it would be useful to have.

My concerns are:

  a.) Why does this include the new makefile in every directory's Makefile,
      even the reference documentation Makefiles ?

  b.) The gtester documentation says that the output can be converted to HTML
      using the gtester-report utility... how come the added Makefile here
      includes a heap of inline xml and translates this to a sort of JUnit
      format ?

  c.) I'm curious how this works, where does the HTML output go ?

      With this applied, can I simply run 'make test-report' and
      there will be a directory created with HTML that I can look at
      using my browser locally ?

      In other words, how accessible is this feature to developers ?

  d.) Finally, I think this should be automated further, can these
      rules be added in an .m4 macro ?

      I wonder if there already is an m4 macro for this in gnome-common,
      otherwise I would suspect it would be a welcome macro in gnome-common
      (this way other modules can easily benefit from this too, without
      adding their own custom makefile, it should be shared).

      I think the best way to implement this would be to create an m4 macro
      in gnome-common and add a single line to EDS's configure.ac (instead
      of modifying every Makefile and including a custom makefile, I think
      it should be implied by any makefile defining a TESTS variable, or
      perhaps a GTESTS variable understood by an m4 macro).

I'm not against it, but the above are my basic concerns.

>Why does this include the new makefile in every directory's Makefile,       even the reference documentation Makefiles ?

Could not found a better way to do this (otherwise 'make test-report' in root dir will fail), suggestions welcome

>how come the added Makefile here includes a heap of inline xml and translates this to a sort of JUnit format

It runs every test, produces output for every test and later combines in one big gtester xml. Then gtester-report produces html report. 

Last step is producing JUnit compatible report using xslt. This step is actually not required and can easily be omitted.

>I'm curious how this works, where does the HTML output go
Stored in current directory and can be used as "artifact" in CI (for instance, Jenkins) - or as a report for maintainer/committer. 

> how accessible is this feature to developers?
Instead of reading the whole output or test-suite.log files in each test dir, the report merges all info about the test - result, error message, test duration, messages etc. After running this report several time anyone will notice that some tests are irregularly fail and pass (test-client-suppress-notifications and test-client-get-revision). This kind of statistic (ideally across several commits/runs) shows that these tests should either be disabled or fixed - and this kind of conclusion is not that easy to watching stdout from 'make check'

>Finally, I think this should be automated further, can these rules be added in an .m4 macro
I'm not that good in m4 and Makefiles, this patch is more like a quick solution for eds. Note, that this  patch is mostly a copy of solution used already in dconf and qemu. Of course, a more intelligent and neat solution (ready to use in other project) to have gtester xml output (which is a core point of this patch) is welcome.

Of course, the main point of this patch is to prepare EDS for CI integration - this will unlock several interesting features, like easy patch tests, build failure notifications and constant quality control.

(In reply to comment #4)
> >Why does this include the new makefile in every directory's Makefile,       even the reference documentation Makefiles ?
> 
> Could not found a better way to do this (otherwise 'make test-report' in root
> dir will fail), suggestions welcome
> 
> >how come the added Makefile here includes a heap of inline xml and translates this to a sort of JUnit format
> 
> It runs every test, produces output for every test and later combines in one
> big gtester xml. Then gtester-report produces html report. 
> 
> Last step is producing JUnit compatible report using xslt. This step is
> actually not required and can easily be omitted.

I would think this conversion is a special use-case, personally I don't
like the idea of including that custom case in EDS proper (perhaps you
could write your own conversion script if interacting with third-parting
tooling is important for you).

> 
> >I'm curious how this works, where does the HTML output go
> Stored in current directory and can be used as "artifact" in CI (for instance,
> Jenkins) - or as a report for maintainer/committer. 
> 
> > how accessible is this feature to developers?
> Instead of reading the whole output or test-suite.log files in each test dir,
> the report merges all info about the test - result, error message, test
> duration, messages etc. After running this report several time anyone will
> notice that some tests are irregularly fail and pass
> (test-client-suppress-notifications and test-client-get-revision). This kind of
> statistic (ideally across several commits/runs) shows that these tests should
> either be disabled or fixed - and this kind of conclusion is not that easy to
> watching stdout from 'make check'

In these cases I'm quite certain that the problem has to do with race
conditions in EDS proper (specifically when interacting with the EBookClient
in a non-default thread).

> 
> >Finally, I think this should be automated further, can these rules be added in an .m4 macro
> I'm not that good in m4 and Makefiles, this patch is more like a quick solution
> for eds. Note, that this  patch is mostly a copy of solution used already in
> dconf and qemu. Of course, a more intelligent and neat solution (ready to use
> in other project) to have gtester xml output (which is a core point of this
> patch) is welcome.
> 
> Of course, the main point of this patch is to prepare EDS for CI integration -
> this will unlock several interesting features, like easy patch tests, build
> failure notifications and constant quality control.

I'm not against this in general, and the HTML output does look good.

Perhaps we can run with this in EDS for now as an incubator of sorts
for other GNOME projects, and adapt it into a proper m4 macro as a separate
step (hopefully with a more formal approach using an m4 macro we can
also eliminate the need to modify unrelated Makefiles that dont 
build TESTS targets at all, too).

Created attachment 238692 [details] [review]
Use gtester to run tests - removed xslt convertion

Removed xslt convertion - not really necessary and should be performed on CI's side

(In reply to comment #4)
> >Why does this include the new makefile in every directory's Makefile,
> > even the reference documentation Makefiles ?
> 
> Could not found a better way to do this (otherwise 'make test-report' in root
> dir will fail), suggestions welcome

What about adding it to tests/ folder only? I understand that it's easier to run this in a root directory, but as the most directories are unrelated/unaffected, then it sort of makes sense to run "make test-report" inside tests/ directory.

(In reply to comment #7)
> (In reply to comment #4)
> > >Why does this include the new makefile in every directory's Makefile,
> > > even the reference documentation Makefiles ?
> > 
> > Could not found a better way to do this (otherwise 'make test-report' in root
> > dir will fail), suggestions welcome
> 
> What about adding it to tests/ folder only? I understand that it's easier to
> run this in a root directory, but as the most directories are
> unrelated/unaffected, then it sort of makes sense to run "make test-report"
> inside tests/ directory.

I agree, how about adding the include (and Makefile.gtester) only to
the tests/ subdir ?

Then we avoid all the useless diffs throughout the tree.

We can then include some extra rules in the toplevel Makefile, such as:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
.PHONY:test-report perf-report full-report

test-report perf-report full-report:
  <tab>  $(MAKE) -C tests/ $@
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Just so the tests can be invoked from the toplevel, alternatively,
if we have/need to run tests in other directories, then we could
do something like this in the toplevel makefile instead:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
GTESTER_SUBDIRS = tests directory_one directory_two ...

.PHONY:test-report perf-report full-report

test-report perf-report full-report:
  <tab>  for test in $(GTESTER_SUBDIRS); do
  <tab>    $(MAKE) -C $$test $@;
  <tab>  done
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Of course in the above <tab> should really be tabs, and I think
I got the double escape sequence for '$$test' correctly for the
makefile rule, but it always takes me a couple tries to be sure
I got it right...

I think that the Makefile.gtester itself is a bit crack and
definitely should be improved before we can consider sharing
that with other projects.

Created attachment 238801 [details] [review]
Use gtester to run tests - now in tests only

Thanks for comments, current version affects much less makefiles, .gtester is stored in tests/ dir. Note, that addressbook/backends/google/tests/ contains phonenumber test (but that's another story)

>I think that the Makefile.gtester itself is a bit crack and
definitely should be improved before we can consider sharing
that with other projects.
Actually, this file is (almost) an exact copy from dconf - see [1], so I don't think it would be hard to port this makefile to any other project

I'd really love to see this file to become generic and easily usable in any other project - but this is looks more like an enhancement for gnome-common

[1] https://git.gnome.org/browse/dconf/tree/Makefile.gtester

Some interesting input related to this...

See Mathias Clasen's take on d-d-l:
  https://mail.gnome.org/archives/desktop-devel-list/2013-March/msg00055.html

And the bug report he links to:
  https://bugzilla.gnome.org/show_bug.cgi?id=692125

Thanks for the pointers, Tristan. I'd say to wait for the TAP bug, then return to this again, and possibly re-evaluate.

EDS now uses Installed Tests, so gtester is not required here