Bug 139734 - REGRESSION (r177363): Gtk and Efl testing is broken
Summary: REGRESSION (r177363): Gtk and Efl testing is broken
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on: 139749
Blocks: 139671
  Show dependency treegraph
 
Reported: 2014-12-17 09:54 PST by Alexey Proskuryakov
Modified: 2014-12-18 05:32 PST (History)
4 users (show)

See Also:


Attachments
proposed fix (5.13 KB, patch)
2014-12-17 10:15 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
proposed fix (4.90 KB, patch)
2014-12-17 12:04 PST, Alexey Proskuryakov
simon.fraser: review+
Details | Formatted Diff | Diff
proposed fix (8.09 KB, patch)
2014-12-17 15:07 PST, Alexey Proskuryakov
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2014-12-17 09:54:25 PST
1) On the GTK port, the timeout is 6000ms (release), 12000ms (debug) or 60000ms (if checking memory leaks with valgrind)
 http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/port/gtk.py?rev=177363#L83

So, subtracting 5 seconds is not ok for this port.

 2) r177363 breakes all ports that don't enable supports_per_test_timeout() on Tools/Scripts/webkitpy/port/$port.py
 This is because m_timeout is initialized to 0 <http://trac.webkit.org/browser/trunk/Tools/WebKitTestRunner/TestInvocation.cpp?rev=177363#L96>, and never updated with the port default value because the python scripts don't pass the --timeout argument when the previous is not enabled.
Comment 1 Alexey Proskuryakov 2014-12-17 10:15:45 PST
Created attachment 243442 [details]
proposed fix
Comment 2 Carlos Alberto Lopez Perez 2014-12-17 10:40:53 PST
Comment on attachment 243442 [details]
proposed fix

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

> Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:73
> +        elif self._timeout > 5:

Shouldn't this be 5000 instead of 5? Otherwise we could end with negative values.

> Tools/Scripts/webkitpy/port/gtk.py:95
>  

I would suggest adding 6000 and 2000 (that is, 1 second more than what later is going to be substracted).
Otherwise the timeouts for WTRK and run-webkit-test are going to be the same (and therefore the race condition we talked before would happen). Or I'm wrong?
Comment 3 Alexey Proskuryakov 2014-12-17 11:15:27 PST
> Shouldn't this be 5000 instead of 5? Otherwise we could end with negative values.

Oops, thanks for catching that.

> I would suggest adding 6000 and 2000 (that is, 1 second more than what later is going to be substracted).
> Otherwise the timeouts for WTRK and run-webkit-test are going to be the same (and therefore the race condition we talked before would happen). Or I'm wrong?

Hmm, let me double check the flow of the timeout value.
Comment 4 Alexey Proskuryakov 2014-12-17 12:04:08 PST
Created attachment 243448 [details]
proposed fix

You are right, and I didn't realize that the script timeout was passed through the driver. The correct solution is to pass both timeouts as part of DriverInput. That would likely require updating regression tests, and I'm out of time for further refactoring today, so let's just do the hacky fix.
Comment 5 Alexey Proskuryakov 2014-12-17 13:03:17 PST
Committed <http://trac.webkit.org/r177456>.
Comment 6 WebKit Commit Bot 2014-12-17 14:18:17 PST
Re-opened since this is blocked by bug 139749
Comment 7 Alexey Proskuryakov 2014-12-17 15:07:20 PST
Created attachment 243462 [details]
proposed fix

I missed an important part in the original patch; not sure how tests almost worked.
Comment 8 Alexey Proskuryakov 2014-12-17 15:57:14 PST
Committed <http://trac.webkit.org/r177471>.