Bug 84973

Summary: [GTK] Google tests that time out are leaked
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: pnormand
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch pnormand: review+

Description Carlos Garcia Campos 2012-04-26 10:11:18 PDT
The problem is that run-gtk-test doesn't run the tests directly, it runs run-with-jhbuild, that runs jhbuild-wrapper that runs the test. When a test times out the process spawned by run-gtk-tests is killed, but in this case it's not the test, but one of the helps scripts, see:


\_ python ../Tools/Scripts/run-gtk-tests --skip=only --timeout=0 Programs/TestWebKitAPI/WebKit2/TestNewFirstVisuallyNonEmpt
      \_ [python] <defunct>
      \_ python ../Tools/gtk/run-with-jhbuild Programs/TestWebKitAPI/WebKit2/TestNewFirstVisuallyNonEmptyLayout --gtest_throw
            \_ Programs/TestWebKitAPI/WebKit2/TestNewFirstVisuallyNonEmptyLayout --gtest_throw_on_failure
                  \_ ../WebKitBuild/Release/Programs/WebKitWebProcess 9

In the case of glib tests, gtester is spawned that spawns the test, but the pid of the test is shown on stdou, so run-gtk-test parses that info and kills the test too.

So we have at least two options:

 a) Let run-gtk-test run the test directly instead of using two wrapper scripts, and change the bots to call run-gtk-test with jhbuild-wrapper as I already proposed some time ago.
 b) Now that we are using a common main for all google tests, we can just add a message that shows the pid of the process when it starts. We could even use the same message used by gtester so that we can reuse the code that parses it.

What do you think?
Comment 1 Carlos Garcia Campos 2012-05-07 04:09:35 PDT
I've noticed that we are also leaking the Xvfb process for exactly the same reason, and I'm afraid the same happens with any other processes spawned by run-gtk-test, so I think option a) is the best one in the end.
Comment 2 Carlos Garcia Campos 2012-05-07 04:14:59 PDT
Created attachment 140507 [details]
Patch

This patch moves run-gtk-test to Tools/gtk directory renamed as run-api-tests. Tools/Scripts/run-gtk-test is a simple wrapper that checks whether jhbuild is available or not, so that it runs the whole script with jhbuild if present (instead of running jhbuild for every subprocess of the script). In case of using jhbuild, it uses the jhbuild-wrapper directly instead of run-with-jhbuild to avoid another unnecessary layer.
run-api-tests has been simplified a lot, using common.py to get top_level and build paths, and removing the _create_process helper used to run jhbuild. 
This change should be transparent for users of run-gtk-tests, so we don't need any change in the bots.
Comment 3 Philippe Normand 2012-05-07 10:18:50 PDT
Comment on attachment 140507 [details]
Patch

Looks good, thanks!
Comment 4 Carlos Garcia Campos 2012-05-08 02:54:45 PDT
Committed r116410: <http://trac.webkit.org/changeset/116410>
Comment 5 Philippe Normand 2012-05-08 13:53:23 PDT
(In reply to comment #4)
> Committed r116410: <http://trac.webkit.org/changeset/116410>

Since this patch landed the API tests fail every 2 builds (so, green alternates with red API tests). Traceback:

Traceback (most recent call last):
  File "./Tools/gtk/run-api-tests", line 353, in <module>
    sys.exit(TestRunner(options, args).run_tests())
  File "./Tools/gtk/run-api-tests", line 291, in run_tests
    if not self._setup_testing_environment():
  File "./Tools/gtk/run-api-tests", line 177, in _setup_testing_environment
    if not self._start_accessibility_daemons():
  File "./Tools/gtk/run-api-tests", line 131, in _start_accessibility_daemons
    spi_bus_launcher_path = self._lookup_atspi2_binary('at-spi-bus-launcher')
  File "./Tools/gtk/run-api-tests", line 124, in _lookup_atspi2_binary
    filepath = os.path.join(exec_prefix, path, filename)
  File "/usr/lib/python2.6/posixpath.py", line 67, in join
    elif path == '' or path.endswith('/'):
AttributeError: 'NoneType' object has no attribute 'endswith'
Comment 6 Carlos Garcia Campos 2012-05-08 23:30:37 PDT
Actually it failed all the times, I already fixed it in http://trac.webkit.org/changeset/116412
Comment 7 Philippe Normand 2012-05-09 07:09:58 PDT
(In reply to comment #6)
> Actually it failed all the times, I already fixed it in http://trac.webkit.org/changeset/116412

There is still something wrong going on in the Release bot. Can you check it please?