Closed Bug 860839 Opened 7 years ago Closed 7 years ago

Automagical "test" command for mach

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: Gavin, Assigned: gps)

References

Details

(Whiteboard: [mach])

Attachments

(1 file, 2 obsolete files)

for running tests
i.e. I should be able to "mach test bc", or "mach test M5", etc.
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.
Component: mach → General
Product: Core → Testing
Summary: mach should just accept the same aliases that tbpl uses → Automagical "test" command for mach
Whiteboard: [mach]
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.
Attachment #766367 - Flags: review?(jhammel)
Assignee: nobody → gps
How's this look for an initial version? We can iterate over time,
obviously. Something is better than nothing, methinks.
Attachment #766368 - Flags: review?(jhammel)
Attachment #766368 - Flags: review?(gavin.sharp)
Duplicate of this bug: 880386
Duplicate of this bug: 885577
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!
Attachment #766367 - Flags: review?(jhammel) → review+
(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
OS: Mac OS X → All
Hardware: x86 → All
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.
Attachment #766368 - Flags: review?(jhammel) → review-
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
Attachment #766368 - Flags: review?(gavin.sharp) → feedback+
Depends on: 913231
Comment on attachment 766367 [details] [diff] [review]
Allow mach commands to easily dispatch to other mach commands

Moved to another bug.
Attachment #766367 - Attachment is obsolete: true
Depends on: 913241
Now with a data structure describing test suites.
Attachment #800465 - Flags: review?(jhammel)
Attachment #766368 - Attachment is obsolete: true
Comment on attachment 800465 [details] [diff] [review]
Part 2: Initial implementation of |mach test|

Since jhammel removed himself from CC, reassigning review.
Attachment #800465 - Flags: review?(jhammel) → review?(jmaher)
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
Attachment #800465 - Flags: review?(jmaher) → review+
(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
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f055452c4d9
Status: NEW → ASSIGNED
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/8f055452c4d9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Blocks: 920193
Depends on: 938712
Blocks: 1029005
You need to log in before you can comment on or make changes to this bug.