All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in Firefox 57

Status

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: chutten, Assigned: ahal)

Tracking

(Blocks: 1 bug)

Version 3
mozilla57
Points:
---

Firefox Tracking Flags

(e10s+, firefox57 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
./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?
(Reporter)

Comment 1

2 years ago
ni?ahal on RyanVM's instruction :)
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
Created attachment 8777480 [details]
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/
Attachment #8777480 - Flags: review?(cmanchester)
(Assignee)

Updated

2 years ago
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Flags: needinfo?(ahalberstadt)

Updated

2 years ago
Blocks: 984139
tracking-e10s: --- → +
Attachment #8777480 - Flags: review?(cmanchester) → review+
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".
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Comment 6

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8777480 - Flags: review+ → review?(cmanchester)

Comment 7

2 years ago
mozreview-review
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.
Attachment #8777480 - Flags: review?(cmanchester) → review+
(Assignee)

Comment 8

2 years ago
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.
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?
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 10

a year ago
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 hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8777480 - Attachment is obsolete: true
(Assignee)

Comment 12

a year ago
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).
Flags: needinfo?(ahalberstadt)

Comment 13

a year ago
mozreview-review
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
Attachment #8902423 - Flags: review?(cmanchester) → review+

Comment 14

a year ago
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dcfe07e76532
Forward extra arguments in |mach test| to the underlying test commands, r=chmanchester
Thanks for fixing this Andrew!
Duplicate of this bug: 1388117
https://hg.mozilla.org/mozilla-central/rev/dcfe07e76532
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.