1184405 - Make an association between source files and test files in moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

We can take advantage of the work around file metadata in moz.build (bug 1139218), fine grained test selection on try (bug 1149670), possibly some additional features in moz.build, and possibly additional data sources (static and dynamic analysis, historical data) to automatically run a selection of test automation given a changeset.

I'd like to start by extending moz.build to support associating source files with test files in annotations, and combining this with existing data about test manifests to get something suitable to generate try syntax or drive |mach test| based on a changeset.

Comment 1

[Mike Hommey [:glandium]]

Random thought: you're going to end up with test definitions in moz.build for the association with source files, and test definitions in test manifests...

Comment 2

[Gregory Szorc [:gps]]

I've casually remarked to a few ATeam people that we could potentially roll test manifests into moz.build files. We could probably auto generate test manifests from moz.build files to ease the transition pain, as lots of tools rely on the existence of test manifests.

Comment 3

[Chris Manchester (limited bugmail, email directly)]

It's an idea. The build system already extends manifest syntax in ways that make it hard to do interesting things with manifests, so we could solve that problem.

I guess the kinds of things we rely on test manifest definitions for aren't a perfect fit for this, but they're obviously related.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: imported patch pad1.patch

Bug 1184405 - Add SRC_TEST_DEPS to moz.build to specify test files potentially impacted by source files.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: imported patch pad2.patch

Bug 1184405 - High-level tests for SRC_TEST_DEPS.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Additional context for the commits in comment 4 and comment 5 is that the annotations there assume platforms would be derived from a separate Files() annotation. I'm going write up an outline of the design and solicit feedback at Monday's team meeting.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

The docstring for SRC_TEST_DEPS from comment 4 in case people are interested in commenting without digging through the code:

       """Mapping between files in the tree and tests.

        Patterns (as in Files) are interpreted as keys and values
        to make an association between sets of source files and sets of
        test files. This is intended to be used as a basis for tools
        implementing automated test selection prioritization.

        As an example:

        SRC_TEST_DEPS += [
          ('testing/mochitest/runtests.py', 'testing/mochitest/**'),
        ]

        Will suggest that any mochitest under testing/mochitest may be
        impacted a change to runtests.py.

        Patterns may be specified as keys or values, so:

        SRC_TEST_DEPS += [
          ('netwerk/test/httpserver/**', 'testing/mochitest/tests/Harness_sanity/**'),
        ]

        Will suggest that any file change in the httpd.js directory may be relevant
        to mochitest sanity tests (in both examples, the moz.build is at the root
        of the source tree).
        """

Comment 8

[Gregory Szorc [:gps]]

Setting a needinfo on myself so I remember to look at this.

Comment 9

[Chris Manchester (limited bugmail, email directly)]

I discovered an issue with how I'm reading mozbuilds in the current version... I'll look more into alternatives next week, but that particular approach isn't going to work outside of the contrived test examples.

Comment 10

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/15067/#review14973

I'm still looking this over. But as a counter proposal, I'd like you to think about a "tagging" system for declaring relationships instead.

Instead of defining complex mappings between individual files which must be constructed into a graph or graph-like data structure, which almost certainly requires maintaining bi-directional mappings between nodes, how about instead we define a one to many relationship from file to "tag." Using Files() or even test manifests, we can declare tags/labels for individual files. When a file changes, we see what tags it is associated with then correlate that to test files having the same tag. This is technically much easier to implement. A downside is the relationship between files isn't as strong. And you need to solve naming and all the problems that entails (overly-generic names, collisions, etc). FWIW, we already have a "tags" property in test manifests (see browser/devtools/performance/test/unit/xpcshell.ini). Next step is to associate source files to tags?

Tagging also solves a number of other problems. e.g. if you want to subscribe to be notified when certain files change, you can do that based on tags instead of raw paths. Ditto for associating reviewers with certain files. Much easier to say "I'm interested in all files related to X." This is how our modules system applies to code after all (certain directories belong to certain modules).

I'm not saying we should do tagging. But it feels much easier. I just don't know if it is powerful enough for what you have in mind.

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Thank you for the suggestion. I'm not too worried about the implementation at this point, but I am worried about having something as immediately understandable as possible. Tags measure up well in this regard, and I think we could find ways to express most of our use cases with them. For instance, to provide a default for files that aren't explicitly tagged we could conceivably generate an opaque tag and apply it to the manifests mentioned in mozbuilds along the path from a given file to the root of the source tree.

A few things come to mind that might make tags problematic:

Tags have seen some amount of adoption in manifests for the purpose of running tests, but we only have an implementation in manifestparser based manifests. So we're invited to accept an initial version that only works for manifestsparser based manifests or find another way to fill in for reftest and wpt manifest formats. Basing the implementation on tests the build system knows about doesn't cover 100% of what runs on Treeherder, but it gets us further than manifestparser manifests by covering these two harnesses.

Tags will be a new taxonomy for source files, but we already have a directory structure that contains a lot of information useful to classifying source files, and bugzilla component annotations compose another. Introducing another way to classify source files seems a little gratuitous and potentially confusing.

Using tags invites a coarse classification for source files. This may be acceptable, but if each source file potentially has its own rule for dependent test files, then the proliferation of tag names would get out of hand.

Comment 12

[Chris Manchester (limited bugmail, email directly)]

(In reply to Chris Manchester [:chmanchester] from comment #9)
> I discovered an issue with how I'm reading mozbuilds in the current
> version... I'll look more into alternatives next week, but that particular
> approach isn't going to work outside of the contrived test examples.

The issue I'm coming up against is getting a view of all the tests for a tree without a build (running the step to emit tree metadata seems to require running configure) -- I think I'm going to need to rework this to output tags or patterns (whichever we go with) for a source file and let whatever's consuming this resolve that so tests/suites.

Comment 13

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643396 [details]
MozReview Request: imported patch pad1.patch

Bug 1184405 - Add SRC_TEST_DEPS to moz.build to specify test files potentially impacted by source files.

Comment 14

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643397 [details]
MozReview Request: imported patch pad2.patch

Bug 1184405 - High-level tests for SRC_TEST_DEPS.

Comment 15

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: imported patch pad3.patch

Bug 1184405 - Add a mach command to expose test-deps file info.

This commit exposes test-deps file info as a mach command, and
modifies the test scheme reader to make it filter out unsuitable
contexts when generating TestManifest objects for metadata context.

Comment 16

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: imported patch pad4.patch

Bug 1184405 - Take matches by wildcard pattern into account in the test resolver.

Comment 17

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: imported patch pad5.patch

Bug 1184405 - Use SRC_TEST_DEPS from files changed in the current branch when no other arguments are passed to mach try.

Comment 18

[Chris Manchester (limited bugmail, email directly)]

I threaded this the rest of the way through to feeding |./mach try|. There are a few things in reader.py I'm going to try to work out before requesting review, and I'd like to add support for patterns starting at the root of the source tree, but this is working out pretty well.

Comment 19

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643396 [details]
MozReview Request: imported patch pad1.patch

Bug 1184405 - Add SRC_TEST_TAGS and SRC_TEST_PATTERNS to moz.build to specify test files potentially impacted by source files.

Comment 20

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643397 [details]
MozReview Request: imported patch pad2.patch

Bug 1184405 - High level tests for SRC_TEST_TAGS and SRC_TEST_PATTERNS.

Comment 21

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653812 [details]
MozReview Request: imported patch pad3.patch

Bug 1184405 - Add a mach command to expose test-deps file info.

This commit exposes test-deps file info as a mach command, and
modifies the test scheme reader to make it filter out unsuitable
contexts when generating TestManifest objects for metadata context.

Comment 22

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653813 [details]
MozReview Request: imported patch pad4.patch

Bug 1184405 - Take matches by wildcard pattern into account in the test resolver.

Comment 23

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653814 [details]
MozReview Request: imported patch pad5.patch

Bug 1184405 - Use SRC_TEST_DEPS from files changed in the current branch when no other arguments are passed to mach try.

Comment 24

[Chris Manchester (limited bugmail, email directly)]

We discussed this earlier in the week. Next stop is review.

Comment 25

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643396 [details]
MozReview Request: imported patch pad1.patch

Bug 1184405 - Add annotations for tags, file patterns, and test flavors to moz.build to specify tests potentially impacted by source files.

Comment 26

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643397 [details]
MozReview Request: imported patch pad2.patch

Bug 1184405 - High level tests for SRC_TEST_TAGS, SRC_TEST_PATTERNS and SRC_TEST_FILES.

Comment 27

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653812 [details]
MozReview Request: imported patch pad3.patch

Bug 1184405 - Add a test to fail the build if file patterns are present in test dependency annotations that don't correspond to any files on disk.

Comment 28

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653813 [details]
MozReview Request: imported patch pad4.patch

Bug 1184405 - Add a mach command to expose test-deps file info.

This commit exposes test-deps file info as a mach command, and
modifies the test scheme reader to make it filter out unsuitable
contexts when generating TestManifest objects for metadata context.

Comment 29

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653814 [details]
MozReview Request: imported patch pad5.patch

Bug 1184405 - Take matches by wildcard pattern into account in the test resolver.

Comment 30

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: imported patch pad6.patch

Bug 1184405 - Use file metadata from files changed in the current branch in mach try when no other arguments are present.

Comment 31

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/15069/#review16377

::: python/mozbuild/mozbuild/frontend/context.py:566
(Diff revision 4)
> +        'TEST_PATTERNS': (list, list,

I wonder if the variable should be more explicit in its intent here. e.g. IMPACTED_TEST_PATHS, IMPACTED_TEST_TAGS, IMPACTED_TEST_FLAVORS.

Also, in the past we've used some light magic to reduce the number of variables. e.g.

IMPACTED_TESTS.patterns += [...]
IMPACTED_TESTS.tags += [...]

Also, we typically use StrictOrderingOnAppendList to require that list elements are sorted (for readability).

There's also ContextDerivedTypedListWithItems(Path, List) for lists of paths where the string elements magically get converted into rich types with automagic absolute path resolution, among other things.

::: python/mozbuild/mozbuild/frontend/context.py:1613
(Diff revision 4)
> +    'SRC_TEST_PATTERNS': (TypedList(TestDepEntry), list,

This feels redundant with the Files() variables. Why do we have both?

::: python/mozbuild/mozbuild/frontend/reader.py:1244
(Diff revision 4)
> -    def read_relevant_mozbuilds(self, paths):
> +    def read_relevant_mozbuilds(self, paths, follow_tests=False):

TEST_DIRS needs to die. It was cargo culted from Makefiles when moz.build was invented. I'm pretty sure that 95% of moz.build files defined via TEST_DIRS could have their content folded into the parent moz.build. Literally the only place TEST_DIRS is used by the build system outside of moz.build processing kludgery is in config/rules.mk, where DIRS += $(TEST_DIRS) if ENABLE_TESTS is defined.

What I'm trying to say is eliminating TEST_DIRS would be preferred to adding more code to treat it specially.

Comment 32

[Mike Hommey [:glandium]]

> What I'm trying to say is eliminating TEST_DIRS would be preferred to adding more code to treat it specially

Just don't break --disable-tests in the process.

Comment 33

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/15069/#review16377

> This feels redundant with the Files() variables. Why do we have both?

I think the map-like syntax is a pretty good fit, and will end up being a lot less verbose if we have something like n source files and m test files, m <= n, and a different rule for each source file. But we haven't determined this is an important use case, and having multiple syntaxes for file metadata is bad on its own merits. I'm ok with sticking to Files() syntax for now.

> TEST_DIRS needs to die. It was cargo culted from Makefiles when moz.build was invented. I'm pretty sure that 95% of moz.build files defined via TEST_DIRS could have their content folded into the parent moz.build. Literally the only place TEST_DIRS is used by the build system outside of moz.build processing kludgery is in config/rules.mk, where DIRS += $(TEST_DIRS) if ENABLE_TESTS is defined.
> 
> What I'm trying to say is eliminating TEST_DIRS would be preferred to adding more code to treat it specially.

Recursing into TEST_DIRS isn't strictly necessary -- if we're missing important tests that should get pulled in we can add annotations to express that or find a way to phase out those TEST_DIRS. The code I added to deal with them is pretty bad, I'll remove the feature for now.

Comment 34

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/15069/#review16377

> Recursing into TEST_DIRS isn't strictly necessary -- if we're missing important tests that should get pulled in we can add annotations to express that or find a way to phase out those TEST_DIRS. The code I added to deal with them is pretty bad, I'll remove the feature for now.

Yeah, wholesale removal of TEST_DIRS likely won't fly because there are still some things that happen in Makefile.in/moz.build files in TEST_DIRS and this has a strong possibility to break --disable-tests, as glandium mentioned.

There might be a compromise here. We could move the test manifest variables outside of moz.build files executed from TEST_DIRS. This paves the way for removal of TEST_DIRS without actually doing all of the work. Along the same vein, we have a number of moz.build files that conditionally define test manifest variables. Since test manifests are essentially extensions of moz.build files, I think these variables should be unconditionally defined. The emitter can be smart about reacting to them if --disable-tests is in effect. Both these ideas are unfortunate scope bloat. But the alternatives are all hacky and extra complexity. Welcome to the build system.

Comment 35

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/15069/#review16377

> I wonder if the variable should be more explicit in its intent here. e.g. IMPACTED_TEST_PATHS, IMPACTED_TEST_TAGS, IMPACTED_TEST_FLAVORS.
> 
> Also, in the past we've used some light magic to reduce the number of variables. e.g.
> 
> IMPACTED_TESTS.patterns += [...]
> IMPACTED_TESTS.tags += [...]
> 
> Also, we typically use StrictOrderingOnAppendList to require that list elements are sorted (for readability).
> 
> There's also ContextDerivedTypedListWithItems(Path, List) for lists of paths where the string elements magically get converted into rich types with automagic absolute path resolution, among other things.

I can't find a way to use HierarchicalStringList with ContextDerived, but maybe we can find a way around that...

Comment 36

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643396 [details]
MozReview Request: imported patch pad1.patch

Bug 1184405 - Add a container type to mozbuild with a namedtuple-like interface and typed, mutable fields.

Comment 37

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643397 [details]
MozReview Request: imported patch pad2.patch

Bug 1184405 - Add annotations for tags, file patterns, and test flavors to moz.build to specify tests potentially impacted by source files.

Comment 38

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653812 [details]
MozReview Request: imported patch pad3.patch

Bug 1184405 - High level tests for SRC_TEST_TAGS, SRC_TEST_PATTERNS and SRC_TEST_FILES.

Comment 39

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653813 [details]
MozReview Request: imported patch pad4.patch

Bug 1184405 - Add a test to fail the build if file patterns are present in test dependency annotations that don't correspond to any files on disk.

Comment 40

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653814 [details]
MozReview Request: imported patch pad5.patch

Bug 1184405 - Add a mach command to expose test-deps file info.

This commit exposes test-deps file info as a mach command, and
modifies the test scheme reader to make it filter out unsuitable
contexts when generating TestManifest objects for metadata context.

Comment 41

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8656924 [details]
MozReview Request: imported patch pad6.patch

Bug 1184405 - Take matches by wildcard pattern into account in the test resolver.

Comment 42

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1184405 - Use file metadata from files changed in the current branch in mach try when no other arguments are present. r=gps

Bug 1184405 - Use file metadata from files changed in the current branch in mach try when no other arguments are present.

Comment 43

[Gregory Szorc [:gps]]

Comment on attachment 8643396 [details]
MozReview Request: imported patch pad1.patch

https://reviewboard.mozilla.org/r/15069/#review16715

Nice class. I'm kinda surprised we didn't have this before: this could be quite useful.

::: python/mozbuild/mozbuild/frontend/context.py:49
(Diff revision 5)
> +    __slots__ = []

Nit: __slots__ should be a tuple.

::: python/mozbuild/mozbuild/frontend/context.py:511
(Diff revision 5)
> +        __slots__ = [name for name, _ in fields]

Nit: tuple

Comment 44

[Gregory Szorc [:gps]]

Comment on attachment 8643397 [details]
MozReview Request: imported patch pad2.patch

https://reviewboard.mozilla.org/r/15071/#review16721

This is pretty much a r+. But given the scope of the issues, I'd like to see the final version before granting review.

::: python/mozbuild/mozbuild/frontend/context.py:539
(Diff revision 5)
> +                                                 ('flavors', OrderedStringList)])

I'd feel much better if there were a whitelist of values that could be assigned to "flavors." People can and will make typos and we need a mechanism to detect that.

Unfortunately, I don't think we have a type that screens string values. Typically, we create a type with attributes or keys for the appropriate values and assign to them.

::: python/mozbuild/mozbuild/frontend/context.py:609
(Diff revision 5)
> +        'DEPENDENT_TESTS': (DependentTestsEntry,

Should we bikeshed over the name? How about IMPACTED_TESTS? I think this is clearer.

::: python/mozbuild/mozbuild/frontend/context.py:611
(Diff revision 5)
> +            """File patterns, tags, and flavors for tests relevant to these files.

Please make the description more verbose. Please describe all the attributes and their expected values and use cases.

::: python/mozbuild/mozbuild/frontend/context.py:632
(Diff revision 5)
> +                # There must be a real way to get a srcdir relative path out of this

Context has a `_relsrcdir()`. You should use that (preferably after removing the underscore - watch out for the `relsrcdir` attribute conflict).

::: python/mozbuild/mozbuild/frontend/emitter.py:1288
(Diff revision 5)
> +        if o.objdir:

Why was this changed?

::: python/mozbuild/mozbuild/frontend/reader.py:1367
(Diff revision 5)
> +        # This names the context keys that will end up emitting a test
> +        # manifest.
> +        # This should be extracted from emitter.py so we don't get out of
> +        # sync.

This should definitely be a shared variable somewhere.

::: python/mozbuild/mozbuild/frontend/reader.py:1371
(Diff revision 5)
> +        self._test_manifest_contexts = set([

Pro tip: {x, y} is set literal syntax and can be useful for things like this. It only works in Python 2.7, but we require Python 2.7, so feel free to use it.

::: python/mozbuild/mozbuild/frontend/reader.py:1394
(Diff revision 5)
> +        # Using the emitter here creates a circular import (and maybe
> +        # crosses some abstraction boundaries -- this may need to be reworked),
> +        # but it's a convenient way to get the build system's view of tests.

Yes it is. Ideally we'd process test manifests as soon as they are appended to moz.build variables. The code exists in emitter because when moz.build was new, we barely did any execution at moz.build eval time. We're slowly moving towards more and more code executing during eval and making emitter a thin shim. glandium even had ideas of removing emitter entirely!

I don't want to scope bloat your work to refactor test manifest handling. But it is definitely something we should have a bug and inline comment to said bug to track. It's also something you may want to pick up :) I'm not too keen about this hacky code. But it should save a lot of near term work.

Comment 45

[Gregory Szorc [:gps]]

Comment on attachment 8653812 [details]
MozReview Request: imported patch pad3.patch

https://reviewboard.mozilla.org/r/17533/#review16727

Please fold this commit into the previous one, as tests should land with the code.

::: python/mozbuild/mozbuild/test/frontend/test_reader.py:417
(Diff revision 4)
> +            'tagged/src/bar.jsm',
> +            'tagged/src/submodule/foo.js',

It looks like you forgot to add some files or changes to version control, as there is no reference to tags anywhere in this diff.

Comment 46

[Gregory Szorc [:gps]]

Comment on attachment 8653813 [details]
MozReview Request: imported patch pad4.patch

https://reviewboard.mozilla.org/r/17535/#review16729

::: config/tests/test_mozbuild_reading.py:84
(Diff revision 4)
> +            if isinstance(ctx, Files):

I tend to use early return/continue in these scenarios to avoid excessive indentation and the waterfall look of code.

if not isinstance(ctx, Files):
    continue
    
# else

::: config/tests/test_mozbuild_reading.py:93
(Diff revision 4)
> +                    normalized = p[1:] if p.startswith('/') else os.path.join(relsrcdir, p)

This wouldn't be needed if we were using the magic context derived paths instances. Speaking of that, why didn't you use them?

Comment 47

[Gregory Szorc [:gps]]

Comment on attachment 8653814 [details]
MozReview Request: imported patch pad5.patch

https://reviewboard.mozilla.org/r/17537/#review16731

Comment 48

[Gregory Szorc [:gps]]

Comment on attachment 8656924 [details]
MozReview Request: imported patch pad6.patch

https://reviewboard.mozilla.org/r/18265/#review16733

I'm pretty sure we have test coverage for this function. If so, I insist on adding a test, as the test resolver is quite complicated and needs test coverage to ensure we don't regress.

Comment 49

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/18603/#review16735

::: testing/mach_commands.py:494
(Diff revision 1)
> +                for path, info in files_info.items():
> +                    paths |= info.test_files
> +                    tags |= info.test_tags

Did you forget to do test flavors?

::: testing/tools/autotry/autotry.py:147
(Diff revision 1)
> +            args += [line.strip() for line in other_branches]

The strip here is wonky. Can you filter whitespace as part of parsing command output?

::: testing/tools/autotry/autotry.py:164
(Diff revision 1)
> +            '(last(::. and public())::.) - last(::. and public())',

I think you want:

  ::. and not public()

Comment 50

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/15071/#review16721

> Context has a `_relsrcdir()`. You should use that (preferably after removing the underscore - watch out for the `relsrcdir` attribute conflict).

I don't see a great way to get the rest of the way ```mozpath.join(e.context._relsrcdir(e.full_path), mozpath.basename(e.full_path))``` is it, but that's about as verbose as what's here.

> Why was this changed?

Running emit_from_context in the "TestContextReader" for defaults runs this method, but with an empty config.

Comment 51

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/15069/#review16757

::: python/mozbuild/mozbuild/frontend/context.py:505
(Diff revision 5)
> +    This is preferable to using a HierarchicalStringList in circumstances

... except it's not remotely close to a HierarchicalStringList, since it skips the hierarchical part entirely. Not that I'd complain about that (I hate HierarchicalStringList and want to remote it in the long term), but this comment is not very helpful as it is considering the differences.

Comment 52

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/15071/#review16759

::: python/mozbuild/mozbuild/frontend/context.py:632
(Diff revision 5)
> +                # There must be a real way to get a srcdir relative path out of this

You shouldn't care about getting a srcdir relative path. Most things use absolute paths, and it's perfectly fine (and, in fact, usually preferable).

Comment 53

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/15071/#review16721

> Running emit_from_context in the "TestContextReader" for defaults runs this method, but with an empty config.

How much more code would you need to add to the test in order for this not to be required?

Comment 54

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/18603/#review16763

::: testing/tools/autotry/autotry.py:141
(Diff revision 1)
> +            args = ['git', 'branch', '-a']
> +            all_branches = set(subprocess.check_output(args).splitlines())
> +            args = ['git', 'branch', '-a', '--contains', 'HEAD']
> +            head_branches = set(subprocess.check_output(args).splitlines())
> +            other_branches = all_branches - head_branches
> +            args = ['git', 'diff', '--name-only', 'HEAD', '--not']
> +            args += [line.strip() for line in other_branches]
> +            return subprocess.check_output(args).splitlines()

Other than the fact that this is likely going to be slow (git branch -a --contains is not exactly smart):
- both invocations of git branch will output an asterisk preceding the current branch, that you're not stripping
- git diff commit --not other commits is not a valid command. git-diff doesn't take --not and doesn't take more than two commits.
- Even if it was a valid command, it would miserably fail to do the right thing in many cases because gecko-dev being synchronized from mercurial, it has non-increasing commit dates, which throws git's revision walking off the roof. Also, for the same reason, git branch -a --contains might be off too. See this thread: http://marc.info/?l=git&m=143218896817198&w=2 for an example. There are more things that break.

All in all, it might be better to /not/ have support for git than have one that fails randomly.

Comment 55

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/15071/#review16759

> You shouldn't care about getting a srcdir relative path. Most things use absolute paths, and it's perfectly fine (and, in fact, usually preferable).

We want this as output in json-mozbuildinfo on hg.m.o at some point, so we want topsrcdir relative paths.

Comment 56

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/15071/#review16721

> How much more code would you need to add to the test in order for this not to be required?

"TestContextReader" isn't a test, emit_from_context is run with an empty config while extracting files metadata.

Comment 57

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/18603/#review16763

> Other than the fact that this is likely going to be slow (git branch -a --contains is not exactly smart):
> - both invocations of git branch will output an asterisk preceding the current branch, that you're not stripping
> - git diff commit --not other commits is not a valid command. git-diff doesn't take --not and doesn't take more than two commits.
> - Even if it was a valid command, it would miserably fail to do the right thing in many cases because gecko-dev being synchronized from mercurial, it has non-increasing commit dates, which throws git's revision walking off the roof. Also, for the same reason, git branch -a --contains might be off too. See this thread: http://marc.info/?l=git&m=143218896817198&w=2 for an example. There are more things that break.
> 
> All in all, it might be better to /not/ have support for git than have one that fails randomly.

Yes this code is mostly wishful thinking and I do not intend to land it. I guess "--not" with multiple arguments having the result I'm hoping when I test this is just undocumented/undefined behavior.

Comment 58

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643396 [details]
MozReview Request: imported patch pad1.patch

Bug 1184405 - Add a container type to mozbuild with a namedtuple-like interface and typed, mutable fields. r=gps

Comment 59

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643396 [details]
MozReview Request: imported patch pad1.patch

Bug 1184405 - Add a container type to mozbuild with a namedtuple-like interface and typed, mutable fields. r=gps

Comment 60

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643397 [details]
MozReview Request: imported patch pad2.patch

Bug 1184405 - Add annotations for tags, file patterns, and test flavors to moz.build to specify tests potentially impacted by source files.
* * *
Bug 1184405 - High level tests for SRC_TEST_TAGS, SRC_TEST_PATTERNS and SRC_TEST_FILES.

Comment 61

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653812 [details]
MozReview Request: imported patch pad3.patch

imported patch dummy.patch

Comment 62

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653813 [details]
MozReview Request: imported patch pad4.patch

Bug 1184405 - Add a test to fail the build if file patterns are present in test dependency annotations that don't correspond to any files on disk.

Comment 63

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653814 [details]
MozReview Request: imported patch pad5.patch

Bug 1184405 - Add a mach command to expose test-deps file info.

This commit exposes test-deps file info as a mach command, and
modifies the test scheme reader to make it filter out unsuitable
contexts when generating TestManifest objects for metadata context.

Comment 64

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8656924 [details]
MozReview Request: imported patch pad6.patch

Bug 1184405 - Take matches by wildcard pattern into account in the test resolver.

Comment 65

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8658442 [details]
MozReview Request: Bug 1184405 - Use file metadata from files changed in the current branch in mach try when no other arguments are present. r=gps

Bug 1184405 - Use file metadata from files changed in the current branch in mach try when no other arguments are present.

Comment 66

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643396 [details]
MozReview Request: imported patch pad1.patch

Bug 1184405 - Add a container type to mozbuild with a namedtuple-like interface and typed, mutable fields. r=gps

Comment 67

[Gregory Szorc [:gps]]

Comment on attachment 8653813 [details]
MozReview Request: imported patch pad4.patch

https://reviewboard.mozilla.org/r/17535/#review16853

Comment 68

[Gregory Szorc [:gps]]

Comment on attachment 8656924 [details]
MozReview Request: imported patch pad6.patch

https://reviewboard.mozilla.org/r/18265/#review16855

Comment 69

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8643397 [details]
MozReview Request: imported patch pad2.patch

https://reviewboard.mozilla.org/r/15071/#review16919

The code itself seems fine.  But I am missing some context about how this is intended to work in practice.  And I learned some things about the Files() context!

::: python/mozbuild/mozbuild/frontend/context.py:608
(Diff revision 6)
> +        'DEPENDENT_TESTS': (DependentTestsEntry, list,

I'm confused as to how this will work in practice.

Surely the current behaviour is that *any* file could impact *any* test.  So how does one transition to the behaviour you are specifying that *this* file could impact *these* tests?  Waving hands around notation: per test, the former set contains the latter set.  I see some default logic, but I cannot understand how this can work.

::: python/mozbuild/mozbuild/frontend/context.py:674
(Diff revision 6)
> +            if Files.VARIABLES[k][0] is DependentTestsEntry:

Wow, this line is hard to understand.  Indexing with [0] is fragile, and you're testing object equality...

Also, why does this appear to not be conditional on `finalized`?

::: python/mozbuild/mozbuild/frontend/context.py:700
(Diff revision 6)
>          if 'BUG_COMPONENT' in self:

Why are we not serializing this test data?

::: python/mozbuild/mozbuild/frontend/context.py:725
(Diff revision 6)
>          bug_components = Counter()

Why wouldn't we aggregate test information?

::: python/mozbuild/mozbuild/testing.py:230
(Diff revision 6)
> +class TestManifestDefinitions(object):

For next time: this extraction would have made a nice pre: patch, since it is mechanical.

::: python/mozbuild/mozbuild/testing.py:274
(Diff revision 6)
> +    reftest_flavors = ['crashtest', 'reftest']

nit: tuple.

::: python/mozbuild/mozbuild/testing.py:277
(Diff revision 6)
> +    web_platform_tests_flavors =['web-platform-tests',]

nit: tuple, whitespace.

Comment 70

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/15071/#review16919

> Wow, this line is hard to understand.  Indexing with [0] is fragile, and you're testing object equality...
> 
> Also, why does this appear to not be conditional on `finalized`?

FINAL is used to prevent values from getting overwritten when they're mentioned in multiple contexts. We only add to each set of tests, tags, flavors as we go, so I don't think it's necessary here.

Comment 71

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/15071/#review16919

Thank you for the review. I hope my comment below gives some useful context. I have some other potentially relevant ideas about test prioritization in blog-post form at http://chmanchester.github.io/blog/2015/08/06/defining-semi-automatic-test-prioritization/.

> I'm confused as to how this will work in practice.
> 
> Surely the current behaviour is that *any* file could impact *any* test.  So how does one transition to the behaviour you are specifying that *this* file could impact *these* tests?  Waving hands around notation: per test, the former set contains the latter set.  I see some default logic, but I cannot understand how this can work.

How this works in practice depends on the tools we build around it. The last commit in this series extends |./mach try| to look at files_info for files changed locally and pushes relevant tests to try. To give a concrete example, if I were to change toolkit/components/osfile/NativeOSFileInternals.cpp, running |./mach try -p all| would run tests from toolkit/components/osfile/tests/mochi/chrome.ini and toolkit/components/osfile/tests/xpcshell/xpcshell.ini on all platforms because those manifests are mentioned in relevant moz.build file (and no other annotations are present for this file). If other tests are likely to fail when this file changes that can be added to a moz.build file in a DEPENDENT_TESTS annotation.

I think |./mach test| integration could also be useful to sanity check something locally (particularly if someone is working on something out of their usual area and it's not clear what tests to run). I'm still looking into what's feasible, but I'd like to incorporate this into an optional "tiered" mode for try that runs tests in stages from just those in files_info on a few platforms to a full set, giving a good idea of whether a change is safe to land without many hours of running tests after it's clearly not.

> Why are we not serializing this test data?

I meant to ask about this -- the "asdict" method doesn't seem to have any callers. But I don't see any reason not to serialize this.

> Why wouldn't we aggregate test information?

I don't think we need to have a way to aggregate these outside of set union in the iadd overload, but we might need to revisit this.

Comment 72

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/15071/#review17099

::: python/mozbuild/mozbuild/frontend/emitter.py:995
(Diff revision 6)
> -        for path in context.get("WEB_PLATFORM_TESTS_MANIFESTS", []):
> +        for flavor in TestManifestDefinitions.web_platform_tests_flavors:
> +            for path in context.get("%s_MANIFESTS" % flavor.upper().replace('-', '_'), []):

This sort of thing isn't easy to test, but I verified locally that all-tests.json isn't changed by this commit.

Comment 73

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/18603/#review16735

> Did you forget to do test flavors?

I think I need to change |./mach try|'s interaction with chunking before selecting an entire flavor is likely to work, that will need to be a follow up.

Comment 74

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/15071/#review16721

> Should we bikeshed over the name? How about IMPACTED_TESTS? I think this is clearer.

I'm ok with IMPACTED_TESTS. TESTS might be enough, but not sure.

Comment 75

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643396 [details]
MozReview Request: imported patch pad1.patch

Bug 1184405 - Add a container type to mozbuild with a namedtuple-like interface and typed, mutable fields. r=gps

Comment 76

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643397 [details]
MozReview Request: imported patch pad2.patch

Bug 1184405 - Add annotations for tags, file patterns, and test flavors to moz.build to specify tests potentially impacted by source files.
* * *
Bug 1184405 - High level tests for SRC_TEST_TAGS, SRC_TEST_PATTERNS and SRC_TEST_FILES.

Comment 77

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653812 [details]
MozReview Request: imported patch pad3.patch

imported patch dummy.patch

Comment 78

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653813 [details]
MozReview Request: imported patch pad4.patch

Bug 1184405 - Add a test to fail the build if file patterns are present in test dependency annotations that don't correspond to any files on disk.

Comment 79

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653814 [details]
MozReview Request: imported patch pad5.patch

Bug 1184405 - Add a mach command to expose test-deps file info.

This commit exposes test-deps file info as a mach command, and
modifies the test scheme reader to make it filter out unsuitable
contexts when generating TestManifest objects for metadata context.

Comment 80

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8656924 [details]
MozReview Request: imported patch pad6.patch

Bug 1184405 - Take matches by wildcard pattern into account in the test resolver.

Comment 81

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8658442 [details]
MozReview Request: Bug 1184405 - Use file metadata from files changed in the current branch in mach try when no other arguments are present. r=gps

Bug 1184405 - Use file metadata from files changed in the current branch in mach try when no other arguments are present.

Comment 82

[Gregory Szorc [:gps]]

Comment on attachment 8643397 [details]
MozReview Request: imported patch pad2.patch

https://reviewboard.mozilla.org/r/15071/#review17721

Almost there. I'd still like to see the final version before granting r+.

::: python/mozbuild/mozbuild/frontend/context.py:533
(Diff revision 7)
> +OrderedFlavorList = TypedList(Enum(*TestManifestDefinitions.all_test_flavors()),

Nit: rename to OrderedTestFlavorList

::: python/mozbuild/mozbuild/frontend/context.py:674
(Diff revision 7)
> +            if k == 'IMPACTED_TESTS':
> +                self.test_files |= set(mozpath.relpath(e.full_path, e.context.config.topsrcdir)
> +                                       for e in v.files)
> +                self.test_tags |= set(v.tags)
> +                self.test_flavors |= set(v.flavors)
> +                continue

Why isn't this after the "if k in self.finalized" check? As written, FINAL isn't honored for this variable.

::: python/mozbuild/mozbuild/frontend/context.py:675
(Diff revision 7)
> +                self.test_files |= set(mozpath.relpath(e.full_path, e.context.config.topsrcdir)
> +                                       for e in v.files)

This converts SourcePath instances to str/unicode instances. We should be using SourcePath everywhere.

::: python/mozbuild/mozbuild/frontend/reader.py:1352
(Diff revision 7)
> +                flags += test_ctx_reader.test_defaults_for_path(path, ctxs)

Out of curiosity, what's the performance hit of this? (Run `mach build-backend` a few times and reports the stats it prints.)

::: python/mozbuild/mozbuild/frontend/reader.py:1399
(Diff revision 7)
> +                test_context[key] = ctx[key]

I can argue that you should create a new instance of the value here. But it shouldn't be necessary.

::: python/mozbuild/mozbuild/frontend/reader.py:1403
(Diff revision 7)
> +                        if 'relpath' not in t:
> +                            # wpt manifests do not generate relpaths.
> +                            t['relpath'] = mozpath.relpath(t['path'],
> +                                                           self.config.topsrcdir)

This feels like a bug in wpt manifest handling. File a follow-up and leave a comment?

::: python/mozbuild/mozbuild/testing.py:230
(Diff revision 7)
> +class TestManifestDefinitions(object):

This doesn't need to be a class: all this functionality can be exposed as module-level symbols.

::: python/mozbuild/mozbuild/testing.py:258
(Diff revision 7)
> +    test_manifests = dict(

Nit: We typically use UPPERCASE variable names for constants.

Also consider making this a ReadOnlyDict so others can't mutate it.

Comment 83

[Gregory Szorc [:gps]]

Comment on attachment 8658442 [details]
MozReview Request: Bug 1184405 - Use file metadata from files changed in the current branch in mach try when no other arguments are present. r=gps

https://reviewboard.mozilla.org/r/18603/#review17723

Comment 84

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/15071/#review17721

> This converts SourcePath instances to str/unicode instances. We should be using SourcePath everywhere.

We're intending to consume this from json-mozbuildinfo on hg.m.o, and I would expect those consumers to work with srcdir-relative paths. Am I missing a step here?

> Out of curiosity, what's the performance hit of this? (Run `mach build-backend` a few times and reports the stats it prints.)

I will -- does build-backend call files_info?

Comment 85

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/15071/#review17721

> Why isn't this after the "if k in self.finalized" check? As written, FINAL isn't honored for this variable.

We don't have the overwritting behavior of other flags -- child directories only add to the sets in parent directories, so I don't think we need FINAL (although maybe adding it should be a warning or error).

Comment 86

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643396 [details]
MozReview Request: imported patch pad1.patch

Bug 1184405 - Add a container type to mozbuild with a namedtuple-like interface and typed, mutable fields. r=gps

Comment 87

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643397 [details]
MozReview Request: imported patch pad2.patch

Bug 1184405 - Add annotations for tags, file patterns, and test flavors to moz.build to specify tests potentially impacted by source files. r=gps

Comment 88

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653812 [details]
MozReview Request: imported patch pad3.patch

imported patch dummy.patch

Comment 89

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653813 [details]
MozReview Request: imported patch pad4.patch

Bug 1184405 - Add a test to fail the build if file patterns are present in test dependency annotations that don't correspond to any files on disk. r=gps

Comment 90

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653814 [details]
MozReview Request: imported patch pad5.patch

Bug 1184405 - Add a mach command to expose test-deps file info. r=gps

This commit exposes test-deps file info as a mach command, and
modifies the test scheme reader to make it filter out unsuitable
contexts when generating TestManifest objects for metadata context.

Comment 91

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8656924 [details]
MozReview Request: imported patch pad6.patch

Bug 1184405 - Take matches by wildcard pattern into account in the test resolver. r=gps

Comment 92

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8658442 [details]
MozReview Request: Bug 1184405 - Use file metadata from files changed in the current branch in mach try when no other arguments are present. r=gps

Bug 1184405 - Use file metadata from files changed in the current branch in mach try when no other arguments are present. r=gps

Comment 93

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/15071/#review17721

> I will -- does build-backend call files_info?

No noticable performance change -- 5.5 to 5.7 seconds of wall time with and without the patch.

Comment 94

[Gregory Szorc [:gps]]

Comment on attachment 8643397 [details]
MozReview Request: imported patch pad2.patch

https://reviewboard.mozilla.org/r/15071/#review18231

Comment 95

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1482131f4dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d18678498b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7a0080dfd6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/b15484377bdd
https://hg.mozilla.org/integration/mozilla-inbound/rev/13bb82f721eb

Comment 96

[Chris Manchester (limited bugmail, email directly)]

Holding off on the |./mach try| patch so I can rebase on other pending work.

Comment 97

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out only f7a0080dfd6b in https://hg.mozilla.org/integration/mozilla-inbound/rev/adb0582e983d for SM bustage.

Comment 98

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Whoops, forgot the link to the failures: https://treeherder.mozilla.org/logviewer.html#?job_id=14712953&repo=mozilla-inbound

Comment 99

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5abe4f5d5b9f

Comment 100

[Chris Manchester (limited bugmail, email directly)]

(In reply to Pulsebot from comment #99)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5abe4f5d5b9f

Re-landed disabled on the impacted platforms.

Comment 101

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d1482131f4dd
https://hg.mozilla.org/mozilla-central/rev/f6d18678498b
https://hg.mozilla.org/mozilla-central/rev/b15484377bdd
https://hg.mozilla.org/mozilla-central/rev/13bb82f721eb
https://hg.mozilla.org/mozilla-central/rev/5abe4f5d5b9f

Comment 102

[Chris Manchester (limited bugmail, email directly)]

Attachment: Use file metadata from files changed in the current branch in mach try when no other arguments are present

This is the rebase on top of bug 1193215 et al. There was a substantial change needed to autotry.py, so running it by jgraham before landing

Comment 103

[James Graham [:jgraham]]

Comment on attachment 8666893 [details] [diff] [review]
Use file metadata from files changed in the current branch in mach try when no other arguments are present

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

::: testing/mach_commands.py
@@ +578,5 @@
>          if kwargs["push"] and at.find_uncommited_changes():
>              print('ERROR please commit changes before continuing')
>              sys.exit(1)
>  
> +        if not any([kwargs["paths"], kwargs["tests"], kwargs["tags"]]):

Maybe write as

if not any(kwargs[item] for item in ["paths", "tests", "tags"]):

?

@@ +583,1 @@
>              changed_files = at.find_changed_files()

Can we move the body of this if statement into a method that returns paths,tags?

::: testing/tools/autotry/autotry.py
@@ +230,5 @@
>  
> +                if flavor in ["crashtest", "reftest"]:
> +                    manifest_relpath = os.path.relpath(t['manifest'], self.topsrcdir)
> +                    paths_by_flavor[flavor].add(os.path.dirname(manifest_relpath))
> +                elif 'dir_relpath' in t:

I;m not really sure what the point of this change is. Is there a reason that just passing the provided path through to the harness doesn't work?

Comment 104

[Chris Manchester (limited bugmail, email directly)]

(In reply to James Graham [:jgraham] from comment #103)
> Comment on attachment 8666893 [details] [diff] [review]
> Use file metadata from files changed in the current branch in mach try when
> no other arguments are present
> 
> Review of attachment 8666893 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/mach_commands.py
> @@ +578,5 @@
> >          if kwargs["push"] and at.find_uncommited_changes():
> >              print('ERROR please commit changes before continuing')
> >              sys.exit(1)
> >  
> > +        if not any([kwargs["paths"], kwargs["tests"], kwargs["tags"]]):
> 
> Maybe write as
> 
> if not any(kwargs[item] for item in ["paths", "tests", "tags"]):
> 
> ?
> 
> @@ +583,1 @@
> >              changed_files = at.find_changed_files()
> 
> Can we move the body of this if statement into a method that returns
> paths,tags?

Ok.

> 
> ::: testing/tools/autotry/autotry.py
> @@ +230,5 @@
> >  
> > +                if flavor in ["crashtest", "reftest"]:
> > +                    manifest_relpath = os.path.relpath(t['manifest'], self.topsrcdir)
> > +                    paths_by_flavor[flavor].add(os.path.dirname(manifest_relpath))
> > +                elif 'dir_relpath' in t:
> 
> I;m not really sure what the point of this change is. Is there a reason that
> just passing the provided path through to the harness doesn't work?

I'm allowing "paths" to include wildcards, so the "startswith" check fails. The nested loop is a little bit odd anyway, we only need up to one path for each test we selected.

Comment 105

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643396 [details]
MozReview Request: imported patch pad1.patch

imported patch pad1.patch

Comment 106

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8643397 [details]
MozReview Request: imported patch pad2.patch

imported patch pad2.patch

Comment 107

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653812 [details]
MozReview Request: imported patch pad3.patch

imported patch pad3.patch

Comment 108

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653813 [details]
MozReview Request: imported patch pad4.patch

imported patch pad4.patch

Comment 109

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8653814 [details]
MozReview Request: imported patch pad5.patch

imported patch pad5.patch

Comment 110

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8656924 [details]
MozReview Request: imported patch pad6.patch

imported patch pad6.patch

Comment 111

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8658442 [details]
MozReview Request: Bug 1184405 - Use file metadata from files changed in the current branch in mach try when no other arguments are present. r=gps

Bug 1184405 - Use file metadata from files changed in the current branch in mach try when no other arguments are present. r=gps

Comment 112

[James Graham [:jgraham]]

Comment on attachment 8658442 [details]
MozReview Request: Bug 1184405 - Use file metadata from files changed in the current branch in mach try when no other arguments are present. r=gps

https://reviewboard.mozilla.org/r/18603/#review18619

::: testing/tools/autotry/autotry.py:247
(Diff revision 5)
> +        # Removes paths redundant to test selection in the given path set.

This seems fine, but I wonder if it needs to rely on the input paths. With, admittedly, a little more complexity, we could make it just compute the shortest path prefixes that cover all the tests; something like:

```
import os
from collections import deque

input_paths = ["layout", "layout/base", "dom/canvas", "dom/indexeddb/tests", "dom/indexeddb"]

in_input = object()

# First build up a tree of paths represented by nested dicts where each
# path in the input is marked by an entry with key in_input
root = {}
for path in input_paths:
    components = deque(path.split(os.path.sep))
    data = root
    while True:
        try:
            component = components.popleft()
        except IndexError:
            break

        if component not in data:
            data[component] = {}
        data = data[component]

    if data != root:
        data[in_input] = True



# Then use a depth first traversal of the tree stopping when we encounter
# a leaf that was in the input

node = ("", root)
stack = []
to_visit = [node]

rv = []

while to_visit:
    current = to_visit.pop()
    if current is None:
        stack.pop()
        continue

    stack.append(current)
    to_visit.append(None)
    name, node = current
    if in_input in node:
        # This is a leaf node, so serialise
        rv.append(os.path.sep.join(item[0] for item in stack[1:]))
    else:
        for child in node.iteritems():
            to_visit.append(child)

print rv
```
But idk if that's a big enough win.

Comment 113

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd0373aafc35

Comment 114

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fd0373aafc35