Bug 158476

Summary: [GTK] Use a longer timeout for glib unit tests
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, lforschler, mcatanzaro
Priority: P2 Keywords: Gtk
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch darin: review+

Description Carlos Garcia Campos 2016-06-07 05:06:39 PDT
The timeout is supposed to be per test case, but in the case of GLib tests it affects all the tests cases of the same test program. Some test programs like TestLoaderClient, that have a lot of test cases, often time out in the bots because the timeout is not enough to run all the tests cases. So, we should use a longer timeout for GLib tests (for example, timeout * 2).
Comment 1 Carlos Garcia Campos 2016-06-07 05:08:34 PDT
Created attachment 280695 [details]
Patch
Comment 2 Michael Catanzaro 2016-06-07 07:08:11 PDT
Comment on attachment 280695 [details]
Patch

I don't like this hack; we should instead mark affected tests as SLOW.
Comment 3 Carlos Garcia Campos 2016-06-07 10:55:12 PDT
(In reply to comment #2)
> Comment on attachment 280695 [details]
> Patch
> 
> I don't like this hack; we should instead mark affected tests as SLOW.

This is not a hack, we are using the same timeout value to measure different things. At the beginning, all google tests were run like GLib tests, the binary was run with all the tests, but we changed that and we run the same binary with the a filter for every test case. That's possible with google tests because there's a way to get a list of test cases-. That's not possible for GLib tests, and now we have the same timeout value that is supposed to be a test case timeout used for tests cases in case of google and test suites in case of GLib. It's not that TestLoaderClient is slow, and not that we are using a higher value for slow tests, it's just that it runs a lot of test cases. The timout for a single tests case is usually enough to run all TestLoaderClient tests cases, but not always, which means that in the bots it sometimes times out, and it sometimes passes. So it makes sense to use to use a higher timeout for GLib tests and 20 instead of 10 should be enough and still short enough.
Comment 4 Michael Catanzaro 2016-06-07 11:34:09 PDT
(In reply to comment #3)
> That's not
> possible for GLib tests

Can't we use 'gtester -p'?

>, and now we have the same timeout value that is
> supposed to be a test case timeout used for tests cases in case of google
> and test suites in case of GLib. It's not that TestLoaderClient is slow, and
> not that we are using a higher value for slow tests, it's just that it runs
> a lot of test cases. The timout for a single tests case is usually enough to
> run all TestLoaderClient tests cases, but not always, which means that in
> the bots it sometimes times out, and it sometimes passes. So it makes sense
> to use to use a higher timeout for GLib tests and 20 instead of 10 should be
> enough and still short enough.

Yes, I understand, but I don't think a hardcoded, arbitrary 2 is a good way to handle this. We're going to have to find that in a couple years once a test program requires even longer to run. Marking the affected tests as SLOW seems cleaner to me. Anyway, I didn't give r- so you can push ahead if you still disagree.
Comment 5 Carlos Garcia Campos 2016-06-07 23:41:50 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > That's not
> > possible for GLib tests
> 
> Can't we use 'gtester -p'?

You can run tests cases independently with -p, but as I said you can't get the list of tests cases to run, so the test script would need to know in advance the name of all tests cases for all test suites.

> >, and now we have the same timeout value that is
> > supposed to be a test case timeout used for tests cases in case of google
> > and test suites in case of GLib. It's not that TestLoaderClient is slow, and
> > not that we are using a higher value for slow tests, it's just that it runs
> > a lot of test cases. The timout for a single tests case is usually enough to
> > run all TestLoaderClient tests cases, but not always, which means that in
> > the bots it sometimes times out, and it sometimes passes. So it makes sense
> > to use to use a higher timeout for GLib tests and 20 instead of 10 should be
> > enough and still short enough.
> 
> Yes, I understand, but I don't think a hardcoded, arbitrary 2 is a good way
> to handle this. We're going to have to find that in a couple years once a
> test program requires even longer to run. Marking the affected tests as SLOW
> seems cleaner to me. Anyway, I didn't give r- so you can push ahead if you
> still disagree.

All timeout values we use are arbitrary, so the 10 seconds timeout we use for every specific test case is as arbitrary as using 20 for tests suites. If we mark TestLoaderClient as a slow now (which is not slow at all), we will have the same problem eventually if we add more tests cases to other tests, so using a different timeout for test suites than for tests cases also prevents this from happening. So, yes, I still disagree.
Comment 6 Carlos Garcia Campos 2016-06-07 23:48:18 PDT
Committed r201795: <http://trac.webkit.org/changeset/201795>