[webkit-reviews] review granted: [Bug 125386] [GTK] Run each gtest subtest separately instead of in one go : [Attachment 218657] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Gustavo Noronha (kov)
<gns at gnome.org>'s request for review:
Bug 125386: [GTK] Run each gtest subtest separately instead of in one go
https://bugs.webkit.org/show_bug.cgi?id=125386

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

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


Looks good. Just a few nit-picking code organization suggestions...

> Tools/Scripts/run-gtk-tests:327
>      def _run_test_google(self, test_program):

Maybe rename this to _run_google_test_suite?

> Tools/Scripts/run-gtk-tests:345
> +	   try:
> +	       output = subprocess.check_output([test_program,
'--gtest_list_tests'])
> +	   except subprocess.CalledProcessError:
> +	       sys.stderr.write("ERROR: could not list available tests for
binary %s.\n" % (test_program))
> +	       sys.stderr.flush()
> +	       return 1
>  
> -	   return self._run_test_command(tester_command, self._options.timeout)

> +	   available_tests = []
> +	   prefix = None
> +	   for line in output.split('\n'):
> +	       if not line.startswith('  '):
> +		   prefix = line
> +		   continue
> +	       else:
> +		   available_tests.append(prefix + line.strip())
> +
> +	   skipped_test_cases = self._test_cases_to_skip(test_program)
> +

Do you mind splitting this off into a helper method?

> Tools/Scripts/run-gtk-tests:352
> +	       if not test in skipped_test_cases:
> +		   test_command = [test_program, '--gtest_filter=%s' % (test)]
> +		   retcode = self._run_test_command(test_command,
self._options.timeout)
> +		   if retcode:
> +		       return retcode
> +

And then this can be split off into a helper called run_google_test