Bug 84695 - [GTK] run-gtk-tests: Use a timeout per test instead of a global timeout
Summary: [GTK] run-gtk-tests: Use a timeout per test instead of a global timeout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2012-04-24 03:14 PDT by Carlos Garcia Campos
Modified: 2012-04-26 07:44 PDT (History)
2 users (show)

See Also:


Attachments
Patch (6.19 KB, patch)
2012-04-24 03:22 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (6.78 KB, patch)
2012-04-26 04:13 PDT, Carlos Garcia Campos
pnormand: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-04-24 03:14:44 PDT
Using something like 10 secs per test, like run-api-tests does, would detect timed out test earlier. It would also allow to continue running other tests instead of aborting the whole script just because one test timed out.
Comment 1 Carlos Garcia Campos 2012-04-24 03:22:55 PDT
Created attachment 138528 [details]
Patch
Comment 2 Philippe Normand 2012-04-24 06:35:43 PDT
Comment on attachment 138528 [details]
Patch

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

Nice stuff! Looks good to me.

> Tools/Scripts/run-gtk-tests:271
> +            if timeout > 0:
> +                alarm(0)

I'm not sure to understand this code. Can you clarify please?

> Tools/Scripts/run-gtk-tests:307
> +        timedout_tests = []

Hum, what about slow_tests?
Comment 3 Carlos Garcia Campos 2012-04-24 07:43:28 PDT
(In reply to comment #2)
> (From update of attachment 138528 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138528&action=review
> 
> Nice stuff! Looks good to me.
> 
> > Tools/Scripts/run-gtk-tests:271
> > +            if timeout > 0:
> > +                alarm(0)
> 
> I'm not sure to understand this code. Can you clarify please?

alarm(0) just resets the alarm so that no new alarms are scheduled.

> > Tools/Scripts/run-gtk-tests:307
> > +        timedout_tests = []
> 
> Hum, what about slow_tests?

I don't think there's any test so slow, in any case we could use a value higher than 10, but I think 10 seconds per test is enough.
Comment 4 Philippe Normand 2012-04-24 08:16:47 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 138528 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=138528&action=review
> > 
> > Nice stuff! Looks good to me.
> > 
> > > Tools/Scripts/run-gtk-tests:271
> > > +            if timeout > 0:
> > > +                alarm(0)
> > 
> > I'm not sure to understand this code. Can you clarify please?
> 
> alarm(0) just resets the alarm so that no new alarms are scheduled.
> 

Ok.

> > > Tools/Scripts/run-gtk-tests:307
> > > +        timedout_tests = []
> > 
> > Hum, what about slow_tests?
> 
> I don't think there's any test so slow, in any case we could use a value higher than 10, but I think 10 seconds per test is enough.

I guess I meant the "timedout_tests" name wasn't great but that's only my opinion. I proposed slow_tests because I couldn't come up with a better name.
Comment 5 Carlos Garcia Campos 2012-04-24 08:21:52 PDT
(In reply to comment #4)
> > > > Tools/Scripts/run-gtk-tests:307
> > > > +        timedout_tests = []
> > > 
> > > Hum, what about slow_tests?
> > 
> > I don't think there's any test so slow, in any case we could use a value higher than 10, but I think 10 seconds per test is enough.
> 
> I guess I meant the "timedout_tests" name wasn't great but that's only my opinion. I proposed slow_tests because I couldn't come up with a better name.

Ah, you meant the name, sorry. Well, there are some tests that are slow, like BackForardList but they don't time out.
Comment 6 Carlos Garcia Campos 2012-04-26 03:25:28 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > > > > Tools/Scripts/run-gtk-tests:307
> > > > > +        timedout_tests = []
> > > > 
> > > > Hum, what about slow_tests?
> > > 
> > > I don't think there's any test so slow, in any case we could use a value higher than 10, but I think 10 seconds per test is enough.
> > 
> > I guess I meant the "timedout_tests" name wasn't great but that's only my opinion. I proposed slow_tests because I couldn't come up with a better name.
> 
> Ah, you meant the name, sorry. Well, there are some tests that are slow, like BackForardList but they don't time out.

btw, it's called @testsTimedOut in run-api-tests, fwiw
Comment 7 Philippe Normand 2012-04-26 03:34:21 PDT
ok then what about timed_out_tests?
Comment 8 Carlos Garcia Campos 2012-04-26 04:13:11 PDT
Created attachment 138972 [details]
Updated patch

Rebased to apply on current git master and renamed timedout_tests to timed_out_tests
Comment 9 Carlos Garcia Campos 2012-04-26 07:44:33 PDT
Committed r115313: <http://trac.webkit.org/changeset/115313>