Bug 199855 – [GTK][WPE][webkitpy] Refactor drivers to use the base driver for setting up the environment and start the drivers.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Carlos Alberto Lopez Perez]

Currently the webkitpy drivers for GTK and WPE use their own versions for setting up the environment and starting/stopping the drivers.

The code can be simplified by using the base driver, and as a benefit we get some fixes like support for setting the profiler environment variables.

[Carlos Alberto Lopez Perez]

Created attachment 374290 [details]
Patch

[Carlos Alberto Lopez Perez]

Comment on attachment 374290 [details]
Patch

Clearing flags on attachment: 374290

Committed r247512: <https://trac.webkit.org/changeset/247512>

[Carlos Alberto Lopez Perez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53199770>

[Michael Catanzaro]

Looks like this broke WPE and GTK API tests:

python ./Tools/Scripts/run-wpe-tests --release
 in dir /home/buildbot/wpe/wpe-linux-64-release-tests/build (timeout 1200 secs)
 watching logfiles {}
 argv: ['python', './Tools/Scripts/run-wpe-tests', '--release']
 environment:
  HOME=/home/buildbot
  JHBUILD_MIRROR=/home/buildbot/wpe/jhbuild-mirror
  LANG=en_US.UTF-8
  LOGNAME=buildbot
  MAIL=/var/mail/buildbot
  PATH=/etc/zipwrapper:/usr/lib/ccache:/usr/lib/gold-ld:/sbin:/usr/sbin:/bin:/usr/bin
  PWD=/home/buildbot/wpe/wpe-linux-64-release-tests/build
  SHELL=/bin/bash
  SHLVL=0
  TERM=dumb
  TZ=PST8PDT
  USER=buildbot
  WAYLAND_DISPLAY=wayland-0
  WEBKIT_CORE_DUMPS_DIRECTORY=/var/www/cores/bb-wpe-release-test-64
  XDG_RUNTIME_DIR=/run/user/1000
 using PTY: True
TEST: ./Tools/glib/../../WebKitBuild/Release/bin/TestWebKitAPI/WPE/TestWebKitUserContentFilterStore...
Traceback (most recent call last):
  File "./Tools/Scripts/run-wpe-tests", line 70, in <module>
    sys.exit(runner.run_tests())
  File "./Tools/glib/api_test_runner.py", line 294, in run_tests
    results = self._run_test(test)
  File "./Tools/glib/api_test_runner.py", line 266, in _run_test
    return self._run_test_glib(test_program)
  File "./Tools/glib/api_test_runner.py", line 167, in _run_test_glib
    return GLibTestRunner(test_program, timeout, is_slow_test, timeout * 10).run(skipped=self._test_cases_to_skip(test_program), env=self._test_env)
  File "./Tools/glib/glib_test_runner.py", line 251, in run
    p = subprocess.Popen(command, stderr=subprocess.PIPE, env=env)
  File "/usr/lib/python2.7/subprocess.py", line 394, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1047, in _execute_child
    raise child_exception
TypeError: execve() arg 3 contains a non-string value
program finished with exit code 1
elapsedTime=0.283977

[Michael Catanzaro]

Reopening.

[Michael Catanzaro]

Reverted r247512 for reason:

Broke API test runners

Committed r247514: <https://trac.webkit.org/changeset/247514>

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #7)
> Reverted r247512 for reason:
> 
> Broke API test runners
> 
> Committed r247514: <https://trac.webkit.org/changeset/247514>

Ok.

This has been caused because variables with type non-string (NoneType) had been passed to the environment on execve().

And this has been caused because both run-gtk-tests and run-webdriver-tests call Driver._setup_environ_for_test() without calling Driver.start() and this causes that some uninitialised variables to go into the environment (like the driver tempdir) since this variables are defined when starting the driver, but not when creating it.

Before this patch, the GTK/WPE drivers were overriding Driver._setup_environ_for_test() and only setting this variables if they were defined (to avoid this crash). But that was causing this variables to be missed completely from the environment (DUMPRENDERTREE_TEMP and XDG_CACHE_HOME). This looks like a bug to me.

I think the fix is starting the driver on this scripts before trying to get its environment.

[Carlos Alberto Lopez Perez]

Created attachment 374310 [details]
Patch

Try again. Change from before: start the driver in run-gtk-tests and run-webdriver-tests before calling Driver._setup_environ_for_test()

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #9)
> Created attachment 374310 [details]
> Patch
> 
> Try again. Change from before: start the driver in run-gtk-tests and
> run-webdriver-tests before calling Driver._setup_environ_for_test()

run-web-platform-tests also needs this. Will fix before landing.

[Carlos Alberto Lopez Perez]

Committed r247550: <https://trac.webkit.org/changeset/247550>

[Carlos Garcia Campos]

Comment on attachment 374310 [details]
Patch

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

> Tools/Scripts/webkitpy/webdriver_tests/webdriver_test_runner.py:63
> +        self._display_driver.start(False, [])

I think this was on purpose, because the display driver (xvfb, weston, etc) is run in _setup_environ_for_test(), we don't really need the http server running for unit tests (nor webdriver tgests).

[Carlos Alberto Lopez Perez]

(In reply to Carlos Garcia Campos from comment #12)
> Comment on attachment 374310 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374310&action=review
> 
> > Tools/Scripts/webkitpy/webdriver_tests/webdriver_test_runner.py:63
> > +        self._display_driver.start(False, [])
> 
> I think this was on purpose, because the display driver (xvfb, weston, etc)
> is run in _setup_environ_for_test(), we don't really need the http server
> running for unit tests (nor webdriver tgests).

Driver.start() starts DumpRenderTree/WebKitTestRunner (not the http server).
But you have a good point that such thing is not needed for this tests.
I will try to fix this.
Thanks for the feedback

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #13)
> (In reply to Carlos Garcia Campos from comment #12)
> > Comment on attachment 374310 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=374310&action=review
> > 
> > > Tools/Scripts/webkitpy/webdriver_tests/webdriver_test_runner.py:63
> > > +        self._display_driver.start(False, [])
> > 
> > I think this was on purpose, because the display driver (xvfb, weston, etc)
> > is run in _setup_environ_for_test(), we don't really need the http server
> > running for unit tests (nor webdriver tgests).
> 
> Driver.start() starts DumpRenderTree/WebKitTestRunner (not the http server).
> But you have a good point that such thing is not needed for this tests.
> I will try to fix this.
> Thanks for the feedback

Will try to fix this in bug 199945