Bug 199855 - [GTK][WPE][webkitpy] Refactor drivers to use the base driver for setting up the environment and start the drivers.
Summary: [GTK][WPE][webkitpy] Refactor drivers to use the base driver for setting up t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Carlos Alberto Lopez Perez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-17 04:52 PDT by Carlos Alberto Lopez Perez
Modified: 2019-07-19 08:53 PDT (History)
11 users (show)

See Also:


Attachments
Patch (16.56 KB, patch)
2019-07-17 05:07 PDT, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (18.32 KB, patch)
2019-07-17 11:10 PDT, Carlos Alberto Lopez Perez
mcatanzaro: review+
cgarcia: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Alberto Lopez Perez 2019-07-17 04:52:47 PDT
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.
Comment 1 Carlos Alberto Lopez Perez 2019-07-17 05:07:45 PDT
Created attachment 374290 [details]
Patch
Comment 2 Carlos Alberto Lopez Perez 2019-07-17 06:00:55 PDT
Comment on attachment 374290 [details]
Patch

Clearing flags on attachment: 374290

Committed r247512: <https://trac.webkit.org/changeset/247512>
Comment 3 Carlos Alberto Lopez Perez 2019-07-17 06:00:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 4 Radar WebKit Bug Importer 2019-07-17 06:03:42 PDT
<rdar://problem/53199770>
Comment 5 Michael Catanzaro 2019-07-17 08:11:54 PDT
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
Comment 6 Michael Catanzaro 2019-07-17 08:12:02 PDT
Reopening.
Comment 7 Michael Catanzaro 2019-07-17 08:44:30 PDT
Reverted r247512 for reason:

Broke API test runners

Committed r247514: <https://trac.webkit.org/changeset/247514>
Comment 8 Carlos Alberto Lopez Perez 2019-07-17 11:01:17 PDT
(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.
Comment 9 Carlos Alberto Lopez Perez 2019-07-17 11:10:35 PDT
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()
Comment 10 Carlos Alberto Lopez Perez 2019-07-17 18:01:10 PDT
(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.
Comment 11 Carlos Alberto Lopez Perez 2019-07-17 18:05:59 PDT
Committed r247550: <https://trac.webkit.org/changeset/247550>
Comment 12 Carlos Garcia Campos 2019-07-18 01:25:45 PDT
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).
Comment 13 Carlos Alberto Lopez Perez 2019-07-18 03:42:53 PDT
(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
Comment 14 Carlos Alberto Lopez Perez 2019-07-19 08:53:06 PDT
(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