860839 - Automagical "test" command for mach

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

for running tests

Comment 1

[:Gavin Sharp [email: [e-mail]]]

i.e. I should be able to "mach test bc", or "mach test M5", etc.

Comment 2

[Gregory Szorc [:gps]]

Since we don't have a bug on file to track implementing |mach test|, I'm going to use this bug to track that. We can start with a best effort for what's possible now and add more magic later when it's possible. I think having bc, M5, etc as special recognized targets is a good idea.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Allow mach commands to easily dispatch to other mach commands

For |mach test| I'm going to have a mach command dispatch to another
mach command from the same process. The logic in this patch is mostly
copied from main.py. I kinda feel bad for that. But, it works. It's all
in the mach core, so we can clean this up later, IMO.

Comment 4

[Gregory Szorc [:gps]]

Attachment: Part 2: Initial implementation of |mach test|

How's this look for an initial version? We can iterate over time,
obviously. Something is better than nothing, methinks.

Comment 7

[Jeff Hammel]

Comment on attachment 766367 [details] [diff] [review]
Allow mach commands to easily dispatch to other mach commands

+        if not result:
+            result = 0

nit: i'd do `result = result or 0` but it doesn't matter.  Otherwise looks good!

Comment 8

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #7)
> Comment on attachment 766367 [details] [diff] [review]
> Allow mach commands to easily dispatch to other mach commands
> 
> +        if not result:
> +            result = 0
> 
> nit: i'd do `result = result or 0` but it doesn't matter.  Otherwise looks
> good!

actually, i'd do `return fn(**args) or 0` fwiw

Comment 9

[Jeff Hammel]

Comment on attachment 766368 [details] [diff] [review]
Part 2: Initial implementation of |mach test|

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

Overall fine, except wrt the if tree stuff.  giving an r- for now but could be convinced otherwise.

::: testing/mach_commands.py
@@ +86,5 @@
> +                self._mach_context, test_file=None)
> +
> +        if what == 'mochitest-plain':
> +            return self._mach_context.commands.dispatch('mochitest-plain',
> +                self._mach_context, test_file=None)

I'd really rather prefer to keep the mapping not in an if tree in the function. (I'd also prefer not to have an if tree of this sort, tbh.)  I'd rather have an accessible (global, class-level) dict or dicts of what not:

test_suites = {}
test_suites['crashtest'] = set(['crashtest', 'C', 'Rc', 'RC', 'rc'])
...
test_args = {}
test_args['xpcshell-test'] = {'test_file': 'all'}
... 

rev_test_suites = {}
for key, values in test_suites.items():
    for value in values:
        rev_test_suites[value] = key

...

suite = rev_test_suites.get(what)
if what is None:
    ...
args = {'test_file': None}
args.update(test_suite_args.get(suite, {}))
return self._mach_context.commands.dispatch(suite, self._mach_context, **args)

--

while chunked mochitests and IPC tests don't fit this mold....everything else does, and maybe you're more clever than i and can figure out something there.  Or...you can handle these two cases separately.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 766368 [details] [diff] [review]
Part 2: Initial implementation of |mach test|

seems fine to me, but I shouldn't review this python

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 766367 [details] [diff] [review]
Allow mach commands to easily dispatch to other mach commands

Moved to another bug.

Comment 12

[Gregory Szorc [:gps]]

Attachment: Part 2: Initial implementation of |mach test|

Now with a data structure describing test suites.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 800465 [details] [diff] [review]
Part 2: Initial implementation of |mach test|

Since jhammel removed himself from CC, reassigning review.

Comment 14

[Jeff Hammel]

Comment on attachment 800465 [details] [diff] [review]
Part 2: Initial implementation of |mach test|

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

I'm concerned about this getting out of sync with reality.  That said, we have this problem in about 20 million places in our various codebases, so there's no reason to block on that.

Other than that, lgtm.

::: testing/mach_commands.py
@@ +134,5 @@
> +        else:
> +            for v in TEST_SUITES.values():
> +                if what in v.get('aliases', []):
> +                    suite = v
> +                    break

I would probably make a mapping of lookup = dict(sum([[(name, suite_name) for name in [suite_name] + TEST_SUITES[suite_name].get('aliases', [])] for suite_name in TEST_SUITES])) but this is fine

Comment 15

[Jeff Hammel]

(In reply to Gregory Szorc [:gps] from comment #13)
> Comment on attachment 800465 [details] [diff] [review]
> Part 2: Initial implementation of |mach test|
> 
> Since jhammel removed himself from CC, reassigning review.

Too late! :P

Comment 16

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f055452c4d9

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/8f055452c4d9