1100419 - ./mach mochitest should take --timeout parameter to force lower timeout

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[:Gijs (he/him)]

On my high-end machine and an opt build, I don't want to be waiting 1 minute when doing test runs to see which of N tests time out / fail, every time one does. :-)

Comment 1

[:Gijs (he/him)]

Attachment: add --timeout switch to mochitets,

Comment 2

[:Gijs (he/him)]

Comment on attachment 8523931 [details] [diff] [review]
add --timeout switch to mochitets,

There's trivial and then there's unwarranted optimism...

Comment 3

[:Gijs (he/him)]

Attachment: add --timeout switch to mochitets,

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 8524157 [details] [diff] [review]
add --timeout switch to mochitets,

Review of attachment 8524157 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/mach_commands.py
@@ +569,5 @@
>              "If you have run ./mach package beforehand, you can specify 'dist' to " \
>              "run tests against the distribution bundle's binary.");
>      func = app_override(func)
>  
> +    timeout = CommandArgument('--timeout', default=-1, type=int,

Why not just default this to None so you don't have to tweak runtests.py? (I realize you would not be able to use type=int for that.)

@@ +570,5 @@
>              "run tests against the distribution bundle's binary.");
>      func = app_override(func)
>  
> +    timeout = CommandArgument('--timeout', default=-1, type=int,
> +        help='The per-test timeout time in seconds (default: 60 seconds)');

"timeout time" seems redundant.

Comment 5

[:Gijs (he/him)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> Comment on attachment 8524157 [details] [diff] [review]
> add --timeout switch to mochitets,
> 
> Review of attachment 8524157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/mochitest/mach_commands.py
> @@ +569,5 @@
> >              "If you have run ./mach package beforehand, you can specify 'dist' to " \
> >              "run tests against the distribution bundle's binary.");
> >      func = app_override(func)
> >  
> > +    timeout = CommandArgument('--timeout', default=-1, type=int,
> 
> Why not just default this to None so you don't have to tweak runtests.py? (I
> realize you would not be able to use type=int for that.)

Oh, good idea. However, then don't I need to coerce the argument at some later point if it *was* provided? Or does python do that for me?

> @@ +570,5 @@
> >              "run tests against the distribution bundle's binary.");
> >      func = app_override(func)
> >  
> > +    timeout = CommandArgument('--timeout', default=-1, type=int,
> > +        help='The per-test timeout time in seconds (default: 60 seconds)');
> 
> "timeout time" seems redundant.

Fair!

Comment 6

[(not currently active) Ted Mielczarek]

Yeah, fair point, you'd need to do something like 'int(options.timeout) if options.timeout is not None else None', which is maybe uglier (although constrained to one place).

Comment 7

[:Gijs (he/him)]

using None and not touching runtests.py \o/

remote:   https://hg.mozilla.org/integration/fx-team/rev/09034b8710cb

Comment 8

[:Gijs (he/him)]

Erm, I meant:

remote:   https://hg.mozilla.org/integration/fx-team/rev/6e82407a7c65

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e82407a7c65