1289444 - Can ./mach test --disable-e10s be made to work?

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Chris H-C :chutten]

./mach test --disable-e10s <test>

...will complain  "The test command does not accept the arguments: --disable-e10s". Whereas, if <test> happens to be a mochitest, then

./mach mochitest --disable-e10s <test>

...works just fine and dandily.

I don't suppose we could make ./mach test handle --disable-e10s?

Comment 1

[Chris H-C :chutten]

ni?ahal on RyanVM's instruction :)

Comment 2

[Andrew Halberstadt [:ahal]]

Yeah, we should fix this. But we can't just add --disable-e10s to |mach test|, as some of the subcommands don't support that (remember |mach test| is mostly useful for running multiple test suites at once, and every test suite has a different set of command line flags).

But what we can do is pass any unknown arguments down to the various subcommands and hope that they know how to handle them. The tricky part is what do we do if one subcommand accepts the argument, but another doesn't. I think we should continue to fail in this scenario. But agree that if all subcommands accept --disable-e10s, then it should work.

The ideal scenario is to officially standardize which command line flags a test suite needs to accept. But that is a long way away from happening. I'll leave the needinfo for now to take a look at the code when I have some time.

Comment 3

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1289444 - Forward extra arguments in |mach test| to the underlying test commands,

This will allow developers to pass things like --disable-e10s directly on the |mach test| commandline.
There is no good way to tell which arguments will be accepted by which harness', so any test harness
that does not recognize the argument will simply ignore it. This also means that we can't directly add
the arguments to |mach test| and therefore won't be documented when running |mach help test|.

Review commit: https://reviewboard.mozilla.org/r/69012/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69012/

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8777480 [details]
Bug 1289444 - Forward extra arguments in |mach test| to the underlying test commands,

https://reviewboard.mozilla.org/r/69012/#review66436

::: testing/mach_commands.py:307
(Diff revision 1)
>              suite = TEST_SUITES[suite_name]
>  
>              if 'mach_command' in suite:
>                  res = self._mach_context.commands.dispatch(
>                      suite['mach_command'], self._mach_context,
> -                    **suite['kwargs'])
> +                    argv=extra_args, **suite['kwargs'])

Does this always fail and always produce a reasonable error message if we pass bogus arguments?

If not, I think we're better off special-casing "--disable-e10s".

Comment 5

[Andrew Halberstadt [:ahal]]

No, it'll silently be ignored because of this:
https://dxr.mozilla.org/mozilla-central/source/python/mach/mach/registrar.py#119

Though to be honest, I'd rather WONTFIX this than special case --disable-e10s. I spent a lot of time making sure we don't re-define test harness arguments in mach commands, as I think it tends to lead to a big jumbled mess.

Maybe a compromise would be to log a WARNING about unrecognized arguments. I do agree that failing silently is bad.

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 8777480 [details]
Bug 1289444 - Forward extra arguments in |mach test| to the underlying test commands,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69012/diff/1-2/

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8777480 [details]
Bug 1289444 - Forward extra arguments in |mach test| to the underlying test commands,

https://reviewboard.mozilla.org/r/69012/#review67248

::: python/mach/mach/registrar.py:123
(Diff revision 2)
> +            if unknown:
> +                print("warning: unknown arguments {}".format(
> +                    ', '.join(["'{}'".format(arg) for arg in unknown])))
> +

Can we make this an error (I guess with an option to make it non-fatal if existing callers realy on that)?

I can imagine missing this warning if I typoed an argument and being surprised by the result.

Comment 8

[Andrew Halberstadt [:ahal]]

Yeah, there's not really a good option here :/

I agree with you that the warning could cause confusion. But making it fatal also means that we abort as soon as any harness doesn't accept the argument. Given how non-standard the command line arguments are, I'd put the chances of that happening at very high.

I'm tempted to WONTFIX this until we can actually come up with and enforce a standard test cli, but I guess as long as no other calls to dispatch pass in invalid arguments, it doesn't hurt to throw the error. Dispatch isn't widely used, so should be safe.

Comment 9

[Julian Descottes [:jdescottes]]

Sorry to revive this old bug, but we logged a very similar Bug 1388117 recently about --jsdebugger, and I would love to see this landed :)

(In reply to Andrew Halberstadt [:ahal] from comment #8)
> Yeah, there's not really a good option here :/
> 
> I agree with you that the warning could cause confusion. But making it fatal
> also means that we abort as soon as any harness doesn't accept the argument.
> Given how non-standard the command line arguments are, I'd put the chances
> of that happening at very high.

That's true but this would still allow to pass arguments when running any single test. Even if I know I am running a single mochitest, I tend to use ./mach test rather than ./mach mochitest because it's shorter and also works with any other test. 
(I rarely run ./mach test that spawn different harnesses, so I'm a bit biased)

> 
> I'm tempted to WONTFIX this until we can actually come up with and enforce a
> standard test cli, but I guess as long as no other calls to dispatch pass in
> invalid arguments, it doesn't hurt to throw the error. Dispatch isn't widely
> used, so should be safe.

I think having this patch landed can also encourage standardizing cli test arguments. If users see that an argument is supported by a harness but not by another one, it would be a good incentive to improve the situation.

Overall I don't see any downside with this patch compared to the current situation, provided we apply the suggestion from comment 7 (make unrecognized arguments throw an error).

Andrew: any thoughts about this? Is it still something you would like to land?

Comment 10

[Andrew Halberstadt [:ahal]]

Yeah, sorry this stalled out. I think we should get something landed here, whether it only prints a warning or errors out completely. I guess if Chris was worried we can start with error'ing out (which is still an improvement on the current situation) and then revisit if it's a pita.

I'm not sure if I'll be able to look at this right away, so leaving the needinfo for now.

Comment 11

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1289444 - Forward extra arguments in |mach test| to the underlying test commands,

Review commit: https://reviewboard.mozilla.org/r/174008/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/174008/

Comment 12

[Andrew Halberstadt [:ahal]]

Ah for some reason mozreview lost the previous r+, guess it's been too long. Sorry this got delayed so much. This addresses the (now gone) review comment about turning the warning into an error. I guess it's better than nothing for now, we can maybe tweak how this works in the future (or do a better job consolidating test harness arguments).

Comment 13

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8902423 [details]
Bug 1289444 - Forward extra arguments in |mach test| to the underlying test commands,

https://reviewboard.mozilla.org/r/174008/#review179756

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dcfe07e76532
Forward extra arguments in |mach test| to the underlying test commands, r=chmanchester

Comment 15

[Julian Descottes [:jdescottes]]

Thanks for fixing this Andrew!

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/dcfe07e76532