Summary: | REGRESSION (r177363): Gtk and Efl testing is broken | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||||
Component: | Tools / Tests | Assignee: | Alexey Proskuryakov <ap> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | clopez, commit-queue, glenn, ossy | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=139671 | ||||||||||
Bug Depends on: | 139749 | ||||||||||
Bug Blocks: | 139671 | ||||||||||
Attachments: |
|
Description
Alexey Proskuryakov
2014-12-17 09:54:25 PST
Created attachment 243442 [details]
proposed fix
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? > 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. 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.
Committed <http://trac.webkit.org/r177456>. Re-opened since this is blocked by bug 139749 Created attachment 243462 [details]
proposed fix
I missed an important part in the original patch; not sure how tests almost worked.
Committed <http://trac.webkit.org/r177471>. |