[webkit-reviews] review granted: [Bug 82333] [GTK] Use gtester -s to skip individual test cases instead of unit tests as a whole : [Attachment 134050] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 82333: [GTK] Use gtester -s to skip individual test cases instead of unit
tests as a whole
https://bugs.webkit.org/show_bug.cgi?id=82333

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

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=134050&action=review


Looks good, but consider the small changes below before landing.

> Tools/Scripts/run-gtk-tests:34
> +    class SkippedTest:
> +
> +	   def __init__(self, test, reason, bug=None, test_cases=[]):
> +	       self.test = test

Might as well make this a top-level class. There's only one class in this file,
but as time goes on we'll probably add more and want them to interact.

> Tools/Scripts/run-gtk-tests:230
> +	   self._tests = [test for test in self._tests if
self._should_run_test(test)]

You could write this as: self._tests =
itertools.ifilterfalse(self._should_run_test, self._tests)

> Tools/Scripts/run-gtk-tests:248
> -	       names = [os.path.basename(test) for test in failed_tests]
> +	       names = [test.lstrip(self._programs_path) for test in
failed_tests]

This seems unrelated -- there's a lot of general cleanup in this patch though.

> Tools/Scripts/run-gtk-tests:261
> +	       for skipped in self._skipped_tests:
> +		   sys.stdout.write("%s" % skipped.test)
> +		   if skipped.test_cases:
> +		       sys.stdout.write(" [%s]" % ",
".join(skipped.test_cases))
> +		   sys.stdout.write(": %s " % skipped.reason)
> +		   if skipped.bug is not None:
> +		      
sys.stdout.write("(https://bugs.webkit.org/show_bug.cgi?id=%d)" % skipped.bug)
> +		   sys.stdout.write("\n")

I think it'd make sense to have a SkippedTest.__str__ or SkippedTest.__repr__
that converted the class to a string. Then you would just do
sys.stdout.write("%s" % skipped)