[webkit-reviews] review denied: [Bug 80495] [GTK] race condition in run-gtk-tests : [Attachment 130569] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 80495: [GTK] race condition in run-gtk-tests
https://bugs.webkit.org/show_bug.cgi?id=80495

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

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


Nice work, though I think this could use a little cleanup.

> Tools/Scripts/run-gtk-tests:22
> +import os, sys, time

Each import should have its own line.

> Tools/Scripts/run-gtk-tests:24
> +
> +from gi.repository import Gio, GLib

No newline necessary here.

> Tools/Scripts/run-gtk-tests:88
> +    def _wait_dbus_service_and_run(self, service_name, handler):

_run_command_when_dbus_service_appears?

How are timeouts handled here?

> Tools/Scripts/run-gtk-tests:113
> +	   global timed_out;

Instead of a global variable, why not an instance variable?

> Tools/Scripts/run-gtk-tests:131
> +	   def check_bailout():

You should probably rename this to something like:
"check_if_tests_have_timed_out"

> Tools/Scripts/run-gtk-tests:138
> +	       r = False
> +	       if now - start > TIMEOUT:
> +		   sys.stdout.write("Tests timed out after %d seconds\n" %
TIMEOUT)
> +		   sys.stdout.flush()
> +		   r = True
> +	       return r

You can just use an early return here and avoid the variable:

if now.time() - start <= TIMEOUT:
    return True
sys.stdout.write("Tests timed out after %d seconds\n" % TIMEOUT)
sys.stdout.flush()
return False

> Tools/Scripts/run-gtk-tests:152
> +	       # Make sure the accessibility registry daemon is running.
> +	       a11y_registryd_running = False
> +	       a11y_registryd_path = self._lookup_atspi2_binary(jhbuild_path,
'at-spi2-registryd')
> +	       if a11y_registryd_path:
> +		   try:
> +		       a11y_registryd =
Executive().popen([a11y_registryd_path], env=test_env)
> +		       a11y_registryd_running = True
> +		   except:
> +		       sys.stderr.write("Failed to run the accessibility
registry\n")
> +		       sys.stderr.flush()
> +

This could probably be a helper. You could avoid the comment that way:

def _ensure_accessibility_daemon_is_running(self):

> Tools/Scripts/run-gtk-tests:161
> +		   if check_bailout():
> +		       timed_out = True
> +		       break

This doesn't seem to work in situations where the test never halts, right?