Closed Bug 1209188 Opened 4 years ago Closed 4 years ago

mach test should use mozbuild file_info to automatically run relevant tests

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

Details

Attachments

(1 file)

Bug 1184405 added support for mach try, we can do something similar with mach test.
Assignee: nobody → cmanchester
Bug 1209188 - Add a mode to mach test to run impacted tests according to moz.build dependency info. r=ahal

This modifies the behavior of running |./mach test| with no arguments to run
tests relevant to local file changes, as specified by IMPACTED_TESTS annotations
in moz.build files relevant to the changed files.
Attachment #8667008 - Flags: review?(ahalberstadt)
https://reviewboard.mozilla.org/r/20651/#review18507

::: testing/mach_commands.py:245
(Diff revision 1)
> +            from autotry import AutoTry
> +            at = AutoTry(self.topsrcdir, resolver, self._mach_context)
> +            changed_files = at.find_changed_files()

This obviously isn't related to autotry, we can move it to something like testing/tools/autotry/vcsutil/ if need be.
https://reviewboard.mozilla.org/r/20651/#review18507

> This obviously isn't related to autotry, we can move it to something like testing/tools/autotry/vcsutil/ if need be.

Yeah, it would be nicer.. but doesn't need to be a hill you climb now. Maybe at least add a comment to that affect.

Fwiw, I think we should put stuff like this into something like python/mozversioncontrol. See also bug 1185599.
Attachment #8667008 - Flags: review?(ahalberstadt)
Comment on attachment 8667008 [details]
MozReview Request: Bug 1209188 - Add a mode to mach test to run impacted tests according to moz.build dependency info. r=ahal

https://reviewboard.mozilla.org/r/20651/#review18597

::: testing/mach_commands.py:30
(Diff revision 1)
> +changes with IMPACTED_TESTS specified for those files.

I don't think most people are going to know what you're talking about with IMPACTED_TESTS. Maybe include a link or at least mention that it's defined in moz.build.

::: testing/mach_commands.py:261
(Diff revision 1)
> +            for path, info in files_info.items():

nit: for info in files_info.values()

::: testing/mach_commands.py:270
(Diff revision 1)
> +                # If an entire flavor is requested, don't also run individual
> +                # tests in the flavor.
> +                run_tests = [t for t in run_tests if t['flavor'] not in flavors]

Should this be built in to 'resolve_tests'?

::: testing/mach_commands.py:274
(Diff revision 1)
> +                for flavor in flavors:
> +                    run_tests += list(resolver.resolve_tests(flavor=flavor))

Anyway we could modify resolve_tests so that we can call it with all paths/tags/flavors at once? Might save a couple directory traversals of the tree.

This could be a follow-up too.

::: testing/mach_commands.py
(Diff revision 1)
> -            elif 'make_target' in suite:
> -                res = self._run_make(target=suite['make_target'],
> -                    pass_thru=True)
> -                if res:
> -                    status = res

Is this dead code?
https://reviewboard.mozilla.org/r/20651/#review18597

> Should this be built in to 'resolve_tests'?

The test resolver tends to take its inputs and find tests that match all the conditions in its inputs, so it's not a great fit with IMPACTED_TESTS, which wants tests that match any of its inputs... which leads me to realize there's a bug in this patch when it passes tags and paths at the same time.

We could use a much more flexible test selection that what's offered by the test resolver, but we can work around its limitations fairly easily for now if we can accept the performance penalty of multiple calls.

> Anyway we could modify resolve_tests so that we can call it with all paths/tags/flavors at once? Might save a couple directory traversals of the tree.
> 
> This could be a follow-up too.

I filed bug 1210213 on this.

> Is this dead code?

Yep!
https://reviewboard.mozilla.org/r/20651/#review18507

> Yeah, it would be nicer.. but doesn't need to be a hill you climb now. Maybe at least add a comment to that affect.
> 
> Fwiw, I think we should put stuff like this into something like python/mozversioncontrol. See also bug 1185599.

Works for me, I'll leave a pointer to that bug.
Comment on attachment 8667008 [details]
MozReview Request: Bug 1209188 - Add a mode to mach test to run impacted tests according to moz.build dependency info. r=ahal

Bug 1209188 - Add a mode to mach test to run impacted tests according to moz.build dependency info. r=ahal

This modifies the behavior of running |./mach test| with no arguments to run
tests relevant to local file changes, as specified by IMPACTED_TESTS annotations
in moz.build files relevant to the changed files.
Attachment #8667008 - Flags: review?(ahalberstadt)
Comment on attachment 8667008 [details]
MozReview Request: Bug 1209188 - Add a mode to mach test to run impacted tests according to moz.build dependency info. r=ahal

https://reviewboard.mozilla.org/r/20651/#review18841

Thanks, lgtm!
Attachment #8667008 - Flags: review?(ahalberstadt) → review+
https://hg.mozilla.org/mozilla-central/rev/76af185df32b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.