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.

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...

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.

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.

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

Bug 1184405 - High-level tests for SRC_TEST_DEPS.

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.

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). """

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

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.

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.

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.

(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 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 on attachment 8643397 [details] MozReview Request: imported patch pad2.patch Bug 1184405 - High-level tests for SRC_TEST_DEPS.

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.

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

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

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 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 on attachment 8643397 [details] MozReview Request: imported patch pad2.patch Bug 1184405 - High level tests for SRC_TEST_TAGS and SRC_TEST_PATTERNS.

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 on attachment 8653813 [details] MozReview Request: imported patch pad4.patch Bug 1184405 - Take matches by wildcard pattern into account in the test resolver.

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.

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

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 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 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 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 on attachment 8653814 [details] MozReview Request: imported patch pad5.patch Bug 1184405 - Take matches by wildcard pattern into account in the test resolver.

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

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.

> 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.

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.

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.

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 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 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 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 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 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 on attachment 8656924 [details] MozReview Request: imported patch pad6.patch Bug 1184405 - Take matches by wildcard pattern into account in the test resolver.

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

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 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 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 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 on attachment 8653814 [details] MozReview Request: imported patch pad5.patch https://reviewboard.mozilla.org/r/17537/#review16731

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.

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()

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.

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.

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).

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?

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.

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.

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.

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 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 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 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 on attachment 8653812 [details] MozReview Request: imported patch pad3.patch imported patch dummy.patch

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 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 on attachment 8656924 [details] MozReview Request: imported patch pad6.patch Bug 1184405 - Take matches by wildcard pattern into account in the test resolver.

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 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 on attachment 8653813 [details] MozReview Request: imported patch pad4.patch https://reviewboard.mozilla.org/r/17535/#review16853

Comment on attachment 8656924 [details] MozReview Request: imported patch pad6.patch https://reviewboard.mozilla.org/r/18265/#review16855

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.

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.

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.

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.

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.

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 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 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 on attachment 8653812 [details] MozReview Request: imported patch pad3.patch imported patch dummy.patch

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 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 on attachment 8656924 [details] MozReview Request: imported patch pad6.patch Bug 1184405 - Take matches by wildcard pattern into account in the test resolver.

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 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 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

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?

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 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 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 on attachment 8653812 [details] MozReview Request: imported patch pad3.patch imported patch dummy.patch

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 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 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 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

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 on attachment 8643397 [details] MozReview Request: imported patch pad2.patch https://reviewboard.mozilla.org/r/15071/#review18231

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

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

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

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

(In reply to Pulsebot from comment #99) > https://hg.mozilla.org/integration/mozilla-inbound/rev/5abe4f5d5b9f Re-landed disabled on the impacted platforms.

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

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 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?

(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 on attachment 8643396 [details] MozReview Request: imported patch pad1.patch imported patch pad1.patch

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

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

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

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

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

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 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.