Closed Bug 1155338 Opened 4 years ago Closed 4 years ago

[mochitest] Move all options definitions and intelligent defaults out of mach_commands.py and into the harness proper

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(3 files, 1 obsolete file)

The end goal is to have a single entry point into the test harness when running both locally and from automation. Part of achieving this goal means moving all of the "smart" defaults and logic out of the mach_commands.py and into the harness, leaving mach_commands.py an empty shell.

This bug will remove the option redefinitions and default settings. A future bug will consolidate the commands down to one.
Attached file MozReview Request: bz://1155338/ahal (obsolete) —
/r/7155 - Bug 1154006 - [mach] Ability to lazy load parsers passed in via the @Command decorator, r=gps
/r/7157 - Bug 1155338 - Move mach command arguments into mochitest harness

Pull down these commits:

hg pull -r 3be82560e43047248842bfd419b26d40429f73e8 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8593594 [details]
MozReview Request: bz://1155338/ahal

This works with |mach mochitest-plain| on desktop, but not B2G. Haven't tested Android or any of the other flavors yet. Still a good amount of work to do before it'll be ready for review, but feel free to provide early feedback if you want.

Sorry for the large messy patch. If you want I'll try and split it up into logical commits before the final review.

Some things to notice:
1) mach_commands.py and test harness share same set of arguments now
2) "smart default" logic is mostly moved (or moving) into mochitest_options.py
3) MochitestArgumentParser can dynamically set which "containers" are visible (i.e so b2g args won't show up in --help if you have a desktop build).
4) Individual arguments can be suppressed from help (i.e for things that get set automatically/only required in automation)
Attachment #8593594 - Flags: feedback?(cmanchester)
https://reviewboard.mozilla.org/r/7157/#review5937

::: testing/mochitest/bisection.py
(Diff revision 1)
> -        elif self.result[options.bisectChunk] == "FAIL":
> +        elif self.result[options.bisect_chunk] == "FAIL":

I don't mind _that_ much, but this sort of cleanup could use a seperate patch.

::: testing/mochitest/mach_commands.py
(Diff revision 1)
>  FLAVORS = {
>      'mochitest': 'plain',
> +    'plain': 'plain',
>      'chrome': 'chrome',
>      'browser-chrome': 'browser',
> +    'browser': 'browser',
>      'jetpack-package': 'jetpack-package',
>      'jetpack-addon': 'jetpack-addon',
>      'a11y': 'a11y',
>      'webapprt-chrome': 'webapprt-chrome',

I know it isn't entirely related to this bug, but something makes me feel this dict doesn't entirely need to exist.

::: testing/mochitest/mochitest_options.py
(Diff revision 1)
> -        [["--close-when-done"],
> -         {"action": "store_true",
> -          "dest": "closeWhenDone",
> -          "default": False,
> -          "help": "close the application when tests are done running",
> +        # TODO test_paths only works with the mach commands
> +        [["test_paths"],
> +         {"nargs": "*",
> +          "metavar": "TEST",
> +          "default": None,
> +          "help": "Test to run. Can be a single test file or a directory of tests to (run "
> +                  "recursively). If omitted, the entire suite is run.",
> +          }],

Let's pick our poison: leave it in the mach command or do the work of making this uniform between harnesses. My vote's for the latter, it seems to be the de-facto standard way to specify tests.

::: testing/mochitest/mochitest_options.py
(Diff revision 1)
> +        [["--keep-open"],
> +         {"action": "store_false",
> +          "dest": "close_when_done",
> +          "default": True,
> +          "help": "Always keep the browser open after tests complete.",

This is a change from the default, right? I'm all for it, but we should be prepared for workflows coming out of the woodworks. Which also causes me to wonder whether this breaks running tests through testsuite-targets.mk (I have recent evidence this is actually used).

When we're through I guess invoking from make should be trivial, just like our concise mach command.

::: testing/mochitest/mochitest_options.py
(Diff revision 1)
> +        if build_obj:
> +            args.extra_profile_files.append(os.path.join(build_obj.distdir, 'plugins'))

I wonder if it would be better to have one big "if build_obj:" for the case we have one. It might make mucking with this down the road easier to understand.

::: testing/mochitest/mochitest_options.py
(Diff revision 1)
> +            if hasattr(container, 'defaults'):
> +                defaults.update(container.defaults)

Nit: hasattr checks always strike me as a code smell.

::: testing/mochitest/mochitest_options.py
(Diff revision 1)
>          [["--console-level"],
> -         {"dest": "consoleLevel",
> +         {"dest": "console_level",

Looks like you changed almost all of these to underscores, but not all of them... are you planning to change them all?

::: testing/mochitest/runtests.py
(Diff revision 1)
> +    if runner.message_logger.errors:
> +        result = 1
> +        runner.message_logger.logger.warning("The following tests failed:")
> +        for error in runner.message_logger.errors:
> +            runner.message_logger.logger.log_raw(error)
> +

This will cause failures to be double logged in automation and add some noise to the starring interface.

::: testing/mochitest/mochitest_options.py
(Diff revision 1)
> -class B2GOptions(MochitestOptions):
> +    args = [
> -    b2g_options = [
>          [["--b2gpath"],
>           {"dest": "b2gPath",
> -          "help": "path to B2G repo or qemu dir",
>            "default": None,
> +          "help": "Path to B2G repo or QEMU directory.",
> +          "suppress": True,
>            }],
>          [["--desktop"],
>           {"action": "store_true",
> -          "dest": "desktop",
> -          "help": "Run the tests on a B2G desktop build",
>            "default": False,
> +          "help": "Run the tests on a B2G desktop build.",
> +          "suppress": True,
>            }],

Probably a bit down the road, but it doesn't seem far fetched to merge this with the current tree config by putting it someplace mozharness could read and provide substitutions for. This would help us get a handle on the interface to the harnesses from within mozharness (I'd like to do things like figure out from within mozharness what arguments are accepted by a harness and which arguments need input from automation for the harness to run propertly).

::: testing/mochitest/mach_commands.py
(Diff revision 1)
> -def add_mochitest_general_args(parser):
> -    parser.add_argument(
> -        '--debugger',
> -        '-d',
> -        metavar='DEBUGGER',
> -        help='Debugger binary to run test in. Program name or path.')
> -
> -    parser.add_argument(
> -        '--debugger-args',
> -        metavar='DEBUGGER_ARGS',
> -        help='Arguments to pass to the debugger.')

I always find this code really annoying, great to get rid of it\!

::: testing/mochitest/mach_commands.py
(Diff revision 1)
>          elif suite == 'webapprt-chrome':
>              options.webapprtChrome = True
>              options.app = self.get_webapp_runtime_path()
>              options.browserArgs.append("-test-mode")

The precedence seems inverted between this and incoming kwargs... if I wanted to run these tests and override the appname, I don't think I would be able to, because my value would be overriden here.

We're well on our way here. I have a bunch of nits and such, but once we work out the kinks of various platforms this is pretty much good to go. My main question is whether it's too early to stick some of these definitions in a file somewhere that can be read by mochitest or someone locally can use to provide their own defaults.
Comment on attachment 8593594 [details]
MozReview Request: bz://1155338/ahal

Comments in MR
Attachment #8593594 - Flags: feedback?(cmanchester) → feedback+
https://reviewboard.mozilla.org/r/7157/#review5961

> I don't mind _that_ much, but this sort of cleanup could use a seperate patch.

Yeah, I was planning to move at least this stuff into a separate commit. I actually needed to do this, to make the mach_commands.py and mochitest_options.py arguments consistent. There was a lot of "options.thisChunk = this_chunk". Figured if I have to change one of them, might as well make them underscored.

> I know it isn't entirely related to this bug, but something makes me feel this dict doesn't entirely need to exist.

Yeah, you're probably right. Though then -f mochitest and -f browser-chrome would stop working. I think this is also used for printing in the help. Could possibly make the data structure a bit more concise though.

> The precedence seems inverted between this and incoming kwargs... if I wanted to run these tests and override the appname, I don't think I would be able to, because my value would be overriden here.

Yeah good eye, I hadn't gotten around to this yet, but good to call these out as there are a lot of subtle things I could miss.

> Let's pick our poison: leave it in the mach command or do the work of making this uniform between harnesses. My vote's for the latter, it seems to be the de-facto standard way to specify tests.

Agreed.

> This is a change from the default, right? I'm all for it, but we should be prepared for workflows coming out of the woodworks. Which also causes me to wonder whether this breaks running tests through testsuite-targets.mk (I have recent evidence this is actually used).
> 
> When we're through I guess invoking from make should be trivial, just like our concise mach command.

Honestly, I'm not at all concerned about breaking testsuite-targets.mk. If it were up to me we'd nuke it from the tree and force people to use mach. But if it's easy to not break it, then I'll update that too, good thinking. But yes, this is a change from default (along with a few other things like --autorun and --console-level). But these are things that are easy enough to fix, if it does break someone's workflow, they'd simply have to slightly change the command line they're using. I'll be sure to update the MDN pages on running mochitest so we can refer people to them.

> Looks like you changed almost all of these to underscores, but not all of them... are you planning to change them all?

Yeah, I guess I might as well. The ones that I changed were necessary because mach was overriding them. The remaining ones aren't, but might as well be consistent while I'm at it.

> I wonder if it would be better to have one big "if build_obj:" for the case we have one. It might make mucking with this down the road easier to understand.

Yeah, I thought about doing that too but I'm kind of on the fence about it. Doing that means we could have logic for dealing with a particular argument in two separate places. I wanted to keep all logic for a given arg together.

> Probably a bit down the road, but it doesn't seem far fetched to merge this with the current tree config by putting it someplace mozharness could read and provide substitutions for. This would help us get a handle on the interface to the harnesses from within mozharness (I'd like to do things like figure out from within mozharness what arguments are accepted by a harness and which arguments need input from automation for the harness to run propertly).

Agreed, I'm going to focus on that after this lands though.

> Nit: hasattr checks always strike me as a code smell.

Really, why's that? I don't mind fixing this, just curious.

> This will cause failures to be double logged in automation and add some noise to the starring interface.

Ah, good to know. So sounds like the proper fix is to leave this here and remove the code that does this from mozharness? Or will that change result in shuffling the log around unpredictably?
https://reviewboard.mozilla.org/r/7157/#review5967

> Yeah, I thought about doing that too but I'm kind of on the fence about it. Doing that means we could have logic for dealing with a particular argument in two separate places. I wanted to keep all logic for a given arg together.

Yeah, we can just revisit this if it looks like an improvement later.

> Really, why's that? I don't mind fixing this, just curious.

It's totally subjective, and for all I know the python community agrees I'm wrong, in which case ignore me completely.

But the argument is, we control this class hierarchy, so we can decide what it supports and set appropriate values in a base class (we have None which seems to often mean "not set", which is appropriate here). Hasattr seems hacky because it's like a backdoor to treating every object like a dictionary and disclaiming knowledge about what it supports. I think this leads to code that's obsessed with working in all sorts of situations, at the expense of being understandable and at the expense of enforcing its invariants, ultimately at the expense of having things work like we expect. But I also think static types are a good idea and we're already using python, so I can probably safely be ignored.

> Ah, good to know. So sounds like the proper fix is to leave this here and remove the code that does this from mozharness? Or will that change result in shuffling the log around unpredictably?

There's no equivalent in mozharness -- treeherder takes everything line by line and extracts interesting ones. Locally we regurgitate failures lines so people can see at the end of a long test run which ones failed. So maybe just leave this in mach.
Beh, sorry for the bug spam. I'm working through issues that can only be reproduced when running in automation and forgot I had bzpost enabled. I'll disable it now.
Comment on attachment 8593594 [details]
MozReview Request: bz://1155338/ahal

/r/7157 - Bug 1155338 - Move mach command arguments into mochitest harness, r=chmanchester

Pull down this commit:

hg pull -r b44716da97228a0c8c18ef331e2db30b76eb6c3f https://reviewboard-hg.mozilla.org/gecko/
Attachment #8593594 - Flags: feedback+ → review?(cmanchester)
I moved all of the superficial changes (e.g camelCase to underscore) into a different patch to make this easier to review. Here's a good looking try run (I'll do a more complete one later):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=532139546958

I did a fair amount of testing locally, but still want to do a bit more. It's hard to guarantee I didn't break some obscure combination of options. Also I don't have an Android device currently (mine broke) so I have no way of testing that locally either. I'll get someone to test it out before landing.
(In reply to Chris Manchester [:chmanchester] from comment #6)
> There's no equivalent in mozharness -- treeherder takes everything line by
> line and extracts interesting ones. Locally we regurgitate failures lines so
> people can see at the end of a long test run which ones failed. So maybe
> just leave this in mach.

I know we have separate mach and tbpl log formatters--could we make this a feature of the mach log formatter? (Summarizing errors at the end of a run.)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #18)
> (In reply to Chris Manchester [:chmanchester] from comment #6)
> > There's no equivalent in mozharness -- treeherder takes everything line by
> > line and extracts interesting ones. Locally we regurgitate failures lines so
> > people can see at the end of a long test run which ones failed. So maybe
> > just leave this in mach.
> 
> I know we have separate mach and tbpl log formatters--could we make this a
> feature of the mach log formatter? (Summarizing errors at the end of a run.)

We don't use the mach formatter by default for mochitests. I planned to turn it on, but it was missing stuff I was fairly sure would be considered a regression (like output for when an individual assertion passes). I added some of these things so we could make it the default for xpcshell, but even there I got some negative feedback on the move and ended up showing people how to turn the tbpl formatter back on.
I fixed the issue for now by putting it behind an 'if build_obj'. Handling this in the formatters seems like a good idea, but I'd prefer to leave any mozlog changes as follow-up fodder.
Doing it in a follow-up is absolutely fine. I've also suggested in the past that we should merge the tbpl and mach formatters since people expect consistent output from our test harnesses. (We could handle colorizing output as a special case in the formatter.)
Summarizing failures is already a part of the mach formatter, in that sense the only thing that remains is to turn it on and deal with the fallout. But based on my experience Ted is totally right. Differences between output locally and in automation is just confusing and irritating when it makes changes that aren't obviously an enhancement and directly related to that difference, such as colorization.
Comment on attachment 8593594 [details]
MozReview Request: bz://1155338/ahal

https://reviewboard.mozilla.org/r/7153/#review6405

::: testing/mochitest/mach_commands.py
(Diff revision 2)
> -        failure_file_path = os.path.join(
> -            self.statedir,
> -            'mochitest_failures.json')
> -
> -        if rerun_failures and not os.path.exists(failure_file_path):
> -            print('No failure file present. Did you run mochitests before?')
> -            return 1

All the failure file stuff is gone. I wouldn't be surprised if the rerun failure stuff doesn't work anymore, is that the case?

::: testing/mochitest/mach_commands.py:224
(Diff revision 2)
> +        for k, v in kwargs.iteritems():
> +            setattr(options, k, v)

I think this can just be

    options = Namespace(**kwargs)

::: testing/mochitest/mach_commands.py:278
(Diff revision 2)
> -            if len(
> -                    tests) == 1 and closure_behaviour == 'auto' and suite == 'plain':
> +            # XXX why is this such a special case?
> +            if len(tests) == 1 and options.closeWhenDone and suite == 'plain':
>                  options.closeWhenDone = False

I guess if I'm running a single test I probably want to debug it, so closing the browser would be counter-productice. And I guess whoever added this was working on mochitest-plain.

::: testing/mochitest/mach_commands.py:319
(Diff revision 2)
> -    return parser
> +    mochitest_dir = os.path.join(build_obj.topobjdir, '_tests', 'testing', 'mochitest')
> +    os.chdir(build_obj.topobjdir)

This chdir is new... I'm guessing we need it for the relative import below. Can we avoid it be using an absolute import in this case?

::: testing/mochitest/runtests.py:2602
(Diff revision 2)
> +   

Mozreview is showing extra trailing whitespace here.

::: testing/mochitest/runtests.py:65
(Diff revision 2)
> +try:
> +    from mozbuild.base import MozbuildObject
> +    build_obj = MozbuildObject.from_environment(cwd=here)
> +except ImportError:
> +    build_obj = None

This might start succeeding unexpectedly when mozharness moves in tree. During builds we have "MOZ_AUTOMATION" in the environment we can check for, I don't think we have the same in test jobs, but maybe we should find a better way to check for this.

::: testing/mochitest/mochitest_options.py:334
(Diff revision 2)
> -        [["--run-slower"],
> +        [["--slow"],

Why change the name? I like the old name :P

No real blockers here, but a few things that might want to be addressed. This is a big patch, I'll look it over again on Monday.
Attachment #8593594 - Flags: review?(cmanchester)
https://reviewboard.mozilla.org/r/7153/#review6453

> All the failure file stuff is gone. I wouldn't be surprised if the rerun failure stuff doesn't work anymore, is that the case?

Yep that's the case, but it was actually regressed by bug 1144573. The rerun failures stuff used JSON style manifests which were removed by that bug. I decided to remove the logic for this for now to avoid confusion, and I filed bug 1155231 to add it back in.

> Why change the name? I like the old name :P

The mach command had changed the name from --run-slower to --slow, so I was trying to keep the command line that devs are used to seeing the same. But you're right, --run-slower is much more informative, so I should just use that.

> This might start succeeding unexpectedly when mozharness moves in tree. During builds we have "MOZ_AUTOMATION" in the environment we can check for, I don't think we have the same in test jobs, but maybe we should find a better way to check for this.

I'm not convinced that we'd have a build_obj, unless test and build slaves are being merged? I believe mozharness in-tree will still be using the tests.zip method to get artifacts of the build over to the test slave. Either way, I think we can worry about this when it happens. Other harnesses do the same thing.
Attachment #8593594 - Flags: review?(cmanchester)
Comment on attachment 8593594 [details]
MozReview Request: bz://1155338/ahal

/r/7157 - Bug 1155338 - Move mach command arguments into mochitest harness, r=chmanchester

Pull down this commit:

hg pull -r b79d72f972c2f13e0838b84086588d9f48e1e9e8 https://reviewboard-hg.mozilla.org/gecko/
Hey Geoff, could you do me a favour and try running the android mochitest mach command with the above attachment? It seems to run fine in automation (comment 17), but I don't have an android device I can test this out on locally, and I want to make sure I don't break it for devs. Much appreciated!
Flags: needinfo?(gbrown)
Thanks for checking. It looks like there may be some issues.

gbrown@mozpad:~/src$ ./mach mochitest-plain
Error running mach:

    ['mochitest-plain']

The error occurred in mach itself. This is likely a bug in mach itself or a
fundamental problem with a loaded module.

Please consider filing a bug against mach by going to the URL:

    https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=mach


If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

TypeError: coercing to Unicode: need string or buffer, NoneType found

  File "/home/gbrown/src/python/mach/mach/main.py", line 344, in run
    return self._run(argv)
  File "/home/gbrown/src/python/mach/mach/main.py", line 390, in _run
    args = parser.parse_args(argv)
  File "/usr/lib/python2.7/argparse.py", line 1688, in parse_args
    args, argv = self.parse_known_args(args, namespace)
  File "/usr/lib/python2.7/argparse.py", line 1720, in parse_known_args
    namespace, args = self._parse_known_args(args, namespace)
  File "/usr/lib/python2.7/argparse.py", line 1929, in _parse_known_args
    stop_index = consume_positionals(start_index)
  File "/usr/lib/python2.7/argparse.py", line 1885, in consume_positionals
    take_action(action, args)
  File "/usr/lib/python2.7/argparse.py", line 1794, in take_action
    action(self, namespace, argument_values, option_string)
  File "/home/gbrown/src/python/mach/mach/dispatcher.py", line 203, in __call__
    command_namespace, extra = subparser.parse_known_args(args)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/mochitest_options.py", line 1231, in parse_known_args
    return (self.validate(args), remainder)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/mochitest_options.py", line 1223, in validate
    args = container.validate(self, args, self.context)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/mochitest_options.py", line 1054, in validate
    deviceRoot=options.remoteTestRoot)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/devicemanagerSUT.py", line 49, in __init__
    verstring = self._runCmds([{ 'cmd': 'ver' }])
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/devicemanagerSUT.py", line 151, in _runCmds
    self._sendCmds(cmdlist, outputfile, timeout, retryLimit=retryLimit)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/devicemanagerSUT.py", line 127, in _sendCmds
    self._doCmds(cmdlist, outputfile, timeout)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/devicemanagerSUT.py", line 174, in _doCmds
    self._sock.connect((self.host, int(self.port)))
  File "/usr/lib/python2.7/socket.py", line 224, in meth
    return getattr(self._sock,name)(*args)

I notice we are trying to use SUT here, while the normal behavior for the mach command is to use ADB.
Flags: needinfo?(gbrown)
I also checked on robocop -- that looks like a different problem.

gbrown@mozpad:~/src$ ./mach robocop
/home/gbrown/src/testing/mozbase/mozrunner/mozrunner/utils.py:20: UserWarning: Module moznetwork was already imported from /home/gbrown/objdirs/droid/_tests/testing/mochitest/moznetwork.pyc, but /home/gbrown/src/testing/mozbase/moznetwork is being added to sys.path
  import pkg_resources
/home/gbrown/src/testing/mozbase/mozrunner/mozrunner/utils.py:20: UserWarning: Module manifestparser was already imported from /home/gbrown/src/testing/mozbase/manifestparser/manifestparser/__init__.pyc, but /home/gbrown/src/testing/mozbase/manifestdestiny is being added to sys.path
  import pkg_resources
/home/gbrown/src/testing/mozbase/mozrunner/mozrunner/utils.py:20: UserWarning: Module which was already imported from /home/gbrown/src/python/which/which.pyc, but /usr/local/lib/python2.7/dist-packages is being added to sys.path
  import pkg_resources
Error running mach:

    ['robocop']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

Exception: Binary expected at /home/gbrown/objdirs/droid/dist/bin/fennec does not exist.

  File "/home/gbrown/src/testing/mochitest/mach_commands.py", line 675, in run_robocop
    return mochitest.run_android_test(args)
  File "/home/gbrown/src/testing/mochitest/mach_commands.py", line 294, in run_android_test
    sys.exit(runtestsremote.main(args))
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/runtestsremote.py", line 468, in main
    options = parser.parse_args(args)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/mochitest_options.py", line 1227, in parse_args
    return self.validate(ArgumentParser.parse_args(self, *args, **kwargs))
  File "/usr/lib/python2.7/argparse.py", line 1688, in parse_args
    args, argv = self.parse_known_args(args, namespace)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/mochitest_options.py", line 1231, in parse_known_args
    return (self.validate(args), remainder)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/mochitest_options.py", line 1223, in validate
    args = container.validate(self, args, self.context)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/mochitest_options.py", line 548, in validate
    bin_path = build_obj.get_binary_path()
  File "/home/gbrown/src/python/mozbuild/mozbuild/base.py", line 342, in get_binary_path
    raise Exception('Binary expected at %s does not exist.' % path)
Comment on attachment 8593594 [details]
MozReview Request: bz://1155338/ahal

/r/7157 - Bug 1155338 - Move mach command arguments into mochitest harness, r=chmanchester

Pull down this commit:

hg pull -r 62fed3c3bbba507796c8af2eb4959c93e420fe7d https://reviewboard-hg.mozilla.org/gecko/
Geoff: Thanks a bunch! I fixed those, could you please try once more? Sorry for this, but I don't know how else to test it out. I wouldn't be surprised if there are further errors.

Chris: To see the changes since your last review, choose 2-4 on the diff slider.
No worries! After all, you've helped me lots with b2g testing.

Now I get:

gbrown@mozpad:~/src$ ./mach mochitest-plain
usage: 
    Usage instructions for runtests.py.

    All arguments are optional.
    If --chrome is specified, chrome tests will be run instead of web content tests.
    If --browser-chrome is specified, browser-chrome tests will be run instead of web content tests.
    See <http://mochikit.com/doc/html/MochiKit/Logging.html> for details on the logging levels.
    
mach: error: You must specify either appPath or app


and:

gbrown@mozpad:~/src$ ./mach robocop
 ...
 0:04.46 SUITE_START: MainThread 49
 0:04.83 LOG: MainThread INFO Checking for orphan ssltunnel processes...
 0:04.86 LOG: MainThread INFO Checking for orphan xpcshell processes...
 0:08.71 LOG: MainThread ERROR Automation Error: Exception caught while running tests
Traceback (most recent call last):
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/runtestsremote.py", line 644, in main
    result = mochitest.runTests(options)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/runtests.py", line 2110, in runTests
    return self.runMochitests(options, testsToRun, onLaunch)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/runtests.py", line 2047, in runMochitests
    result = self.doTests(options, onLaunch, testsToRun)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/runtests.py", line 2182, in doTests
    self.manifest = self.buildProfile(options)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/runtestsremote.py", line 177, in buildProfile
    manifest = Mochitest.buildProfile(self, options)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/runtests.py", line 1436, in buildProfile
    self.copyExtraFilesToProfile(options)
  File "/home/gbrown/objdirs/droid/_tests/testing/mochitest/runtests.py", line 879, in copyExtraFilesToProfile
    shutil.copytree(abspath, dest)
  File "/usr/lib/python2.7/shutil.py", line 177, in copytree
    os.makedirs(dst)
  File "/usr/lib/python2.7/os.py", line 157, in makedirs
    mkdir(name, mode)
OSError: [Errno 17] File exists: '/tmp/tmpetfgYT.mozrunner/plugins'
Attachment #8593594 - Flags: review?(cmanchester) → review+
Comment on attachment 8593594 [details]
MozReview Request: bz://1155338/ahal

https://reviewboard.mozilla.org/r/7153/#review6507

Just last nits at this point. r+ assuming android is sorted out.

::: testing/mochitest/mochitest_options.py:1173
(Diff revision 4)
> +        if self.app not in container_map.keys():

Can just be "if self.app not in container_map:"

::: testing/mochitest/mochitest_options.py:1032
(Diff revision 4)
> -        defaults["autorun"] = True
> +            if (options.deviceIP):

Nit: parens not needed.

::: testing/mochitest/runtests.py:2627
(Diff revision 4)
> -    commandline.add_logging_group(parser)
> +    options = parser.parse_args(args)

Is there a difference between this and the way it was before, calling parse_args with no arguments and letting argparse figure out argv?
https://reviewboard.mozilla.org/r/7153/#review6541

::: testing/mochitest/runtests.py:2627
(Diff revision 4)
> -    commandline.add_logging_group(parser)
> +    options = parser.parse_args(args)

Nope, args defaults to sys.argv in the method signature, so it's the same thing. This way consumers could pass in a different command line manually if they wanted.. not that they do. It's just a pattern I've gotten into the habit of doing.
Comment on attachment 8593594 [details]
MozReview Request: bz://1155338/ahal

Looks like I'm getting bit by the mozreview not syncing bug.

Geoff, I dug into those last failures you sent and realized the cause was slightly more fundamental than I was hoping. Basically they stemmed from the fact that both mach and the android mochitest harness were creating separate instances of the MochitestArgumentParser. So the fix was to re-implement the android mochitest mach command to work in the same way as the desktop and b2g ones.

Please review just the second commit:
https://reviewboard.mozilla.org/r/7779/diff/0

This passes on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e14c4df1b15d

I also faked my environment to test the validation part locally, but it would be good if you could test it out locally once more (hopefully for the last time!).
Attachment #8593594 - Flags: review+ → review?(gbrown)
Comment on attachment 8593594 [details]
MozReview Request: bz://1155338/ahal

/r/7157 - Bug 1155338 - Move mach command arguments into mochitest harness, r=chmanchester
/r/7779 - Fix android and robocop mach commands

Pull down these commits:

hg pull -r 0b2215864d61677cf2b7890477ae273b6af03e1b https://reviewboard-hg.mozilla.org/gecko/
https://reviewboard.mozilla.org/r/7779/#review6633

::: testing/mochitest/mach_commands.py
(Diff revision 1)
> -            '--xre-path=' + os.environ.get('MOZ_HOST_BIN'),

Setting --xre-path from MOZ_HOST_BIN is lost -- that seems to be a problem.

::: testing/mochitest/mach_commands.py:640
(Diff revision 1)
> -            '--xre-path=' + os.environ.get('MOZ_HOST_BIN'),
> +            kwargs['robocopIni'] = os.path.join(self.topobjdir, 'build', 'mobile',

--xre-path again

::: testing/mochitest/mach_commands.py
(Diff revision 1)
> -            '--robocop-apk=' +

I think not setting robocop-apk might be a problem too. If I correct for MOZ_HOST_BIN, ./mach robocop seems to run plain mochitests!
Comment on attachment 8593594 [details]
MozReview Request: bz://1155338/ahal

/r/7157 - Bug 1155338 - Move mach command arguments into mochitest harness, r=chmanchester
/r/7779 - Fix android and robocop mach commands

Pull down these commits:

hg pull -r a0e8ecb333afdbb5f7cc9f15c5c1000d1fe2cdfd https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8593594 [details]
MozReview Request: bz://1155338/ahal

/r/7157 - Bug 1155338 - Move mach command arguments into mochitest harness, r=chmanchester
/r/7779 - Fix android and robocop mach commands

Pull down these commits:

hg pull -r 7675e7faba0dfcecda22145f71ee5e55befc5b84 https://reviewboard-hg.mozilla.org/gecko/
Forgot to fix the robocopApk problem, latest review request should fix that too.
Comment on attachment 8593594 [details]
MozReview Request: bz://1155338/ahal

https://reviewboard.mozilla.org/r/7153/#review6719

I tested mochitest-plain, mochitest-chrome, and robocop targets with/without a test path against an android emulator -- all good.
Attachment #8593594 - Flags: review?(gbrown) → review+
Here's a last try run with all the new changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=242fdbc82ff4

The Android 4.0 robocop failures are my fault, but they only run on try and there's a patch to disable them under review, so I'm not too inclined to worry about fixing them. I guess I should wait for them to get disabled before landing to avoid confusion.
Depends on: 1159365
Oops, that robocop failure doesn't require a buildbot-configs change after all.. I just missed a --robocop in the in-tree configs, fixed. There was also a smattering of bitrot, so here's another try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee8ae17e85b4
No longer depends on: 1159365
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ff5b76266d63 - those infinite retries of opt and debug 4.0 mochitests? We do still run debug 4.0 mochitests on non-Try (and I doubt we have enough pandas to afford infinitely retrying mochitests on every -p all -u all Try push even if we didn't).
Hm, I didn't think that was from my patch since it doesn't even get to the runtests step.
Still not entirely sure how choosing pandas work, but looks like adding --dm_trans=sut did the trick:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b7f42239e30
https://hg.mozilla.org/mozilla-central/rev/2a4e56526e67
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1162022
Blocks: 1162226
Filed regression bug 1162362
Blocks: 1162479
Blocks: 1163037
See Also: → 1163755
See Also: → 1163793
See Also: → 1163797
Blocks: 1164003
Attachment #8593594 - Attachment is obsolete: true
Attachment #8620069 - Flags: review+
Attachment #8620070 - Flags: review+
Attachment #8620071 - Flags: review+
Depends on: 1173691
You need to log in before you can comment on or make changes to this bug.