Bug 81415 – [GTK] Allow running run-gtk-tests during 'make distcheck'

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

It's becoming harder and harder to run GTK+ API tests outside of run-gtk-tests. 'make dist' should be able to use run-gtk-tests so that we can rely on some environment variables and Xvfb to be around.

[Martin Robinson]

Created attachment 132395 [details]
Patch

[Martin Robinson]

This bug is on track for WebKitGTK+ 1.8.

[Philippe Normand]

Comment on attachment 132395 [details]
Patch

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)

[Martin Robinson]

Comment on attachment 132395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132395&action=review

>> Source/WebKit/gtk/GNUmakefile.am:-639
>> -	  $(GTESTER_REPORT) test-report.xml > test-report.html ;
> 
> Would it be useful to add support for the test-report generation in run-gtk-tests?

It might be, though I'm not aware of anyone ever using this feature. It could be a feature of run-gtk-tests instead if people are interested.

>> 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.

Okay.

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

Yikes! Thanks.

>> 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.

Fixed.

>> Tools/Scripts/run-gtk-tests:182
>> +                    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)

The problem with sets is that I want to do a substring search. Is it possible easily? I considered using lisp-y list manipulation like map, but this seemed clearer in the end. I have to land this patch now, but if you can think of a better way that involves substrings, I'll gladly update this code in a followup patch.

Thanks for the review!

[Martin Robinson]

Committed r111316: <http://trac.webkit.org/changeset/111316>