[webkit-reviews] review granted: [Bug 81415] [GTK] Allow running run-gtk-tests during 'make distcheck' : [Attachment 132395] Patch

[bugzilla-daemon at webkit.org]

Philippe Normand <pnormand at igalia.com> has granted Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 81415: [GTK] Allow running run-gtk-tests during 'make distcheck'
https://bugs.webkit.org/show_bug.cgi?id=81415

Attachment 132395: Patch
https://bugs.webkit.org/attachment.cgi?id=132395&action=review

------- Additional Comments from Philippe Normand <pnormand at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=132395&action=review


Nice patch! Just a few remarks to consider before landing :)

> Source/WebKit/gtk/GNUmakefile.am:-639
> -	  $(GTESTER) --verbose $$test_options -o test-report.xml $(TEST_PROGS);
\
> -	  $(GTESTER_REPORT) test-report.xml > test-report.html ;

Would it be useful to add support for the test-report generation in
run-gtk-tests?

> Tools/Scripts/run-gtk-tests:122
> +	   if (not spi_bus_launcher_path) or (not spi_registryd_path):

You can just omit the parenthesis here I think.

> Tools/Scripts/run-gtk-tests:126
> +	       uself._ally_bus_launcher =
self._create_process([spi_bus_launcher_path], env=self._test_env)

typo: uself

> Tools/Scripts/run-gtk-tests:139
> +	   if spi_registryd_path:

I don't think this test is needed, it's done in line 122 already.

> Tools/Scripts/run-gtk-tests:182
> +	   tests_to_remove = []
> +	   for test in self._tests:
> +	       for skipped in self._skipped_tests:
> +		   if test.find(skipped) != -1:
> +		       tests_to_remove.append(test)
> +		       continue

Maybe you can simplify this code by using sets, if you can avoid using .find()
to filter, something like:

tests = sets.Set(self._tests)
self._tests = tests.difference(self._skipped_tests)