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

RESOLVED FIXED in Firefox 44

Status

()

Core
Build Config
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: chmanchester, Assigned: chmanchester)

Tracking

(Depends on: 3 bugs)

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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...

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
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.
(Assignee)

Comment 4

2 years ago
Created 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.
(Assignee)

Comment 5

2 years ago
Created attachment 8643397 [details]
MozReview Request: imported patch pad2.patch

Bug 1184405 - High-level tests for SRC_TEST_DEPS.
(Assignee)

Comment 6

2 years ago
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.
(Assignee)

Comment 7

2 years ago
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

2 years ago
Setting a needinfo on myself so I remember to look at this.
Flags: needinfo?(gps)
(Assignee)

Comment 9

2 years ago
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.
(Assignee)

Comment 11

2 years ago
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.
(Assignee)

Comment 12

2 years ago
(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.
(Assignee)

Updated

2 years ago
Assignee: nobody → cmanchester
(Assignee)

Comment 13

2 years ago
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.
(Assignee)

Comment 14

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

Bug 1184405 - High-level tests for SRC_TEST_DEPS.
(Assignee)

Comment 15

2 years ago
Created 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.
(Assignee)

Comment 16

2 years ago
Created attachment 8653813 [details]
MozReview Request: imported patch pad4.patch

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

Comment 17

2 years ago
Created 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.
(Assignee)

Comment 18

2 years ago
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.
(Assignee)

Comment 19

2 years ago
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.
Attachment #8643396 - Attachment description: MozReview Request: Bug 1184405 - Add SRC_TEST_DEPS to moz.build to specify test files potentially impacted by source files. → MozReview Request: Bug 1184405 - Add SRC_TEST_TAGS and SRC_TEST_PATTERNS to moz.build to specify test files potentially impacted by source files.
(Assignee)

Updated

2 years ago
Attachment #8643397 - Attachment description: MozReview Request: Bug 1184405 - High-level tests for SRC_TEST_DEPS. → MozReview Request: Bug 1184405 - High level tests for SRC_TEST_TAGS and SRC_TEST_PATTERNS.
(Assignee)

Comment 20

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

Bug 1184405 - High level tests for SRC_TEST_TAGS and SRC_TEST_PATTERNS.
(Assignee)

Comment 21

2 years ago
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.
(Assignee)

Comment 22

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

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

Comment 23

2 years ago
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.
(Assignee)

Updated

2 years ago
Depends on: 1201693
(Assignee)

Comment 24

2 years ago
We discussed this earlier in the week. Next stop is review.
(Assignee)

Updated

2 years ago
Flags: needinfo?(gps)
(Assignee)

Comment 25

2 years ago
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.
Attachment #8643396 - Attachment description: MozReview Request: Bug 1184405 - Add SRC_TEST_TAGS and SRC_TEST_PATTERNS to moz.build to specify test files potentially impacted by source files. → MozReview Request: Bug 1184405 - Add annotations for tags, file patterns, and test flavors to moz.build to specify tests potentially impacted by source files.
(Assignee)

Updated

2 years ago
Attachment #8643397 - Attachment description: MozReview Request: Bug 1184405 - High level tests for SRC_TEST_TAGS and SRC_TEST_PATTERNS. → MozReview Request: Bug 1184405 - High level tests for SRC_TEST_TAGS, SRC_TEST_PATTERNS and SRC_TEST_FILES.
(Assignee)

Comment 26

2 years ago
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.
(Assignee)

Comment 27

2 years ago
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.
Attachment #8653812 - Attachment description: MozReview Request: Bug 1184405 - Add a mach command to expose test-deps file info. → MozReview Request: 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.
(Assignee)

Comment 28

2 years ago
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.
Attachment #8653813 - Attachment description: MozReview Request: Bug 1184405 - Take matches by wildcard pattern into account in the test resolver. → MozReview Request: Bug 1184405 - Add a mach command to expose test-deps file info.
(Assignee)

Comment 29

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

Bug 1184405 - Take matches by wildcard pattern into account in the test resolver.
Attachment #8653814 - Attachment description: MozReview Request: Bug 1184405 - Use SRC_TEST_DEPS from files changed in the current branch when no other arguments are passed to mach try. → MozReview Request: Bug 1184405 - Take matches by wildcard pattern into account in the test resolver.
(Assignee)

Comment 30

2 years ago
Created attachment 8656924 [details]
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.
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.
(Assignee)

Comment 33

2 years ago
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.
(Assignee)

Updated

2 years ago
Depends on: 1201965
(Assignee)

Comment 35

2 years ago
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...
(Assignee)

Comment 36

2 years ago
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.
Attachment #8643396 - Attachment description: MozReview Request: Bug 1184405 - Add annotations for tags, file patterns, and test flavors to moz.build to specify tests potentially impacted by source files. → MozReview Request: Bug 1184405 - Add a container type to mozbuild with a namedtuple-like interface and typed, mutable fields.
Attachment #8643396 - Flags: review?(gps)
(Assignee)

Comment 37

2 years ago
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.
Attachment #8643397 - Attachment description: MozReview Request: Bug 1184405 - High level tests for SRC_TEST_TAGS, SRC_TEST_PATTERNS and SRC_TEST_FILES. → MozReview Request: Bug 1184405 - Add annotations for tags, file patterns, and test flavors to moz.build to specify tests potentially impacted by source files.
Attachment #8643397 - Flags: review?(gps)
(Assignee)

Updated

2 years ago
Attachment #8653812 - Attachment description: MozReview Request: 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. → MozReview Request: Bug 1184405 - High level tests for SRC_TEST_TAGS, SRC_TEST_PATTERNS and SRC_TEST_FILES.
Attachment #8653812 - Flags: review?(gps)
(Assignee)

Comment 38

2 years ago
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.
(Assignee)

Comment 39

2 years ago
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.
Attachment #8653813 - Attachment description: MozReview Request: Bug 1184405 - Add a mach command to expose test-deps file info. → MozReview Request: 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.
Attachment #8653813 - Flags: review?(gps)
(Assignee)

Comment 40

2 years ago
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.
Attachment #8653814 - Attachment description: MozReview Request: Bug 1184405 - Take matches by wildcard pattern into account in the test resolver. → MozReview Request: Bug 1184405 - Add a mach command to expose test-deps file info.
Attachment #8653814 - Flags: review?(gps)
(Assignee)

Comment 41

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

Bug 1184405 - Take matches by wildcard pattern into account in the test resolver.
Attachment #8656924 - Attachment description: MozReview Request: Bug 1184405 - Use file metadata from files changed in the current branch in mach try when no other arguments are present. → MozReview Request: Bug 1184405 - Take matches by wildcard pattern into account in the test resolver.
Attachment #8656924 - Flags: review?(gps)
(Assignee)

Comment 42

2 years ago
Created 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

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
Attachment #8643396 - Flags: review?(gps) → review+
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.
Attachment #8643397 - Flags: review?(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.
Attachment #8653812 - Flags: review?(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?
Attachment #8653813 - Flags: review?(gps)
Comment on attachment 8653814 [details]
MozReview Request: imported patch pad5.patch

https://reviewboard.mozilla.org/r/17537/#review16731
Attachment #8653814 - Flags: review?(gps) → review+

Updated

2 years ago
Attachment #8656924 - Flags: review?(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.
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()

Updated

2 years ago
Attachment #8643397 - Flags: feedback?(nalexander)
(Assignee)

Updated

2 years ago
Depends on: 1203266
(Assignee)

Comment 50

2 years ago
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.
(Assignee)

Comment 55

2 years ago
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.
(Assignee)

Comment 56

2 years ago
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.
(Assignee)

Comment 57

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8643396 - Attachment description: MozReview Request: Bug 1184405 - Add a container type to mozbuild with a namedtuple-like interface and typed, mutable fields. → MozReview Request: Bug 1184405 - Add a container type to mozbuild with a namedtuple-like interface and typed, mutable fields. r=gps
(Assignee)

Comment 58

2 years ago
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
(Assignee)

Comment 59

2 years ago
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
(Assignee)

Updated

2 years ago
Attachment #8643397 - Flags: feedback?(nalexander)
(Assignee)

Comment 60

2 years ago
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.
(Assignee)

Comment 61

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

imported patch dummy.patch
Attachment #8653812 - Attachment description: MozReview Request: Bug 1184405 - High level tests for SRC_TEST_TAGS, SRC_TEST_PATTERNS and SRC_TEST_FILES. → MozReview Request: imported patch dummy.patch
(Assignee)

Updated

2 years ago
Attachment #8653813 - Flags: review?(gps)
(Assignee)

Comment 62

2 years ago
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.
(Assignee)

Comment 63

2 years ago
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.
(Assignee)

Comment 64

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

Bug 1184405 - Take matches by wildcard pattern into account in the test resolver.
Attachment #8656924 - Flags: review?(gps)
(Assignee)

Comment 65

2 years ago
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.
(Assignee)

Comment 66

2 years ago
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
(Assignee)

Updated

2 years ago
Attachment #8643397 - Flags: feedback?(nalexander)
(Assignee)

Updated

2 years ago
Depends on: 1203686

Updated

2 years ago
Attachment #8653813 - Flags: review?(gps) → review+
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
Attachment #8656924 - Flags: review?(gps) → review+
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.
Attachment #8643397 - Flags: review+
(Assignee)

Comment 70

2 years ago
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.
(Assignee)

Comment 71

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8643397 - Flags: feedback?(nalexander)
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 72

2 years ago
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.
(Assignee)

Comment 73

2 years ago
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.
(Assignee)

Comment 74

2 years ago
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.
(Assignee)

Comment 75

2 years ago
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
(Assignee)

Comment 76

2 years ago
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.
Attachment #8643397 - Flags: review?(gps)
(Assignee)

Comment 77

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

imported patch dummy.patch
(Assignee)

Comment 78

2 years ago
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.
(Assignee)

Comment 79

2 years ago
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.
(Assignee)

Comment 80

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

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

Comment 81

2 years ago
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.
Attachment #8658442 - Flags: review?(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.
Attachment #8643397 - Flags: review?(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
Attachment #8658442 - Flags: review?(gps) → review+
(Assignee)

Comment 84

2 years ago
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?
(Assignee)

Comment 85

2 years ago
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).
(Assignee)

Comment 86

2 years ago
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
(Assignee)

Updated

2 years ago
Attachment #8643397 - Attachment description: MozReview Request: Bug 1184405 - Add annotations for tags, file patterns, and test flavors to moz.build to specify tests potentially impacted by source files. → MozReview Request: Bug 1184405 - Add annotations for tags, file patterns, and test flavors to moz.build to specify tests potentially impacted by source files. r=gps
Attachment #8643397 - Flags: review?(gps)
(Assignee)

Comment 87

2 years ago
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
(Assignee)

Comment 88

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

imported patch dummy.patch
(Assignee)

Updated

2 years ago
Attachment #8653813 - Attachment description: MozReview Request: 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. → MozReview Request: 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
(Assignee)

Comment 89

2 years ago
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
(Assignee)

Comment 90

2 years ago
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.
Attachment #8653814 - Attachment description: MozReview Request: Bug 1184405 - Add a mach command to expose test-deps file info. → MozReview Request: Bug 1184405 - Add a mach command to expose test-deps file info. r=gps
(Assignee)

Updated

2 years ago
Attachment #8656924 - Attachment description: MozReview Request: Bug 1184405 - Take matches by wildcard pattern into account in the test resolver. → MozReview Request: Bug 1184405 - Take matches by wildcard pattern into account in the test resolver. r=gps
(Assignee)

Comment 91

2 years ago
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
(Assignee)

Comment 92

2 years ago
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
Attachment #8658442 - Attachment description: MozReview Request: Bug 1184405 - Use file metadata from files changed in the current branch in mach try when no other arguments are present. → 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
(Assignee)

Comment 93

2 years ago
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
Attachment #8643397 - Flags: review?(gps) → review+

Comment 95

2 years ago
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
(Assignee)

Comment 96

2 years ago
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.
Flags: needinfo?(cmanchester)
Whoops, forgot the link to the failures: https://treeherder.mozilla.org/logviewer.html#?job_id=14712953&repo=mozilla-inbound

Comment 99

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5abe4f5d5b9f
(Assignee)

Comment 100

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

Re-landed disabled on the impacted platforms.
Flags: needinfo?(cmanchester)
(Assignee)

Updated

2 years ago
Depends on: 1208645
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
(Assignee)

Updated

2 years ago
Blocks: 1209188
(Assignee)

Updated

2 years ago
Keywords: leave-open
(Assignee)

Comment 102

2 years ago
Created attachment 8666893 [details] [diff] [review]
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
Attachment #8666893 - Flags: review?(james)
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?
Attachment #8666893 - Flags: review?(james)
(Assignee)

Comment 104

2 years ago
(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.
(Assignee)

Comment 105

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

imported patch pad1.patch
Attachment #8643396 - Attachment description: MozReview Request: Bug 1184405 - Add a container type to mozbuild with a namedtuple-like interface and typed, mutable fields. r=gps → MozReview Request: imported patch pad1.patch
(Assignee)

Updated

2 years ago
Attachment #8643397 - Attachment description: MozReview Request: Bug 1184405 - Add annotations for tags, file patterns, and test flavors to moz.build to specify tests potentially impacted by source files. r=gps → MozReview Request: imported patch pad2.patch
(Assignee)

Comment 106

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

imported patch pad2.patch
(Assignee)

Comment 107

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

imported patch pad3.patch
Attachment #8653812 - Attachment description: MozReview Request: imported patch dummy.patch → MozReview Request: imported patch pad3.patch
(Assignee)

Comment 108

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

imported patch pad4.patch
Attachment #8653813 - Attachment description: MozReview Request: 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 → MozReview Request: imported patch pad4.patch
(Assignee)

Comment 109

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

imported patch pad5.patch
Attachment #8653814 - Attachment description: MozReview Request: Bug 1184405 - Add a mach command to expose test-deps file info. r=gps → MozReview Request: imported patch pad5.patch
(Assignee)

Comment 110

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

imported patch pad6.patch
Attachment #8656924 - Attachment description: MozReview Request: Bug 1184405 - Take matches by wildcard pattern into account in the test resolver. r=gps → MozReview Request: imported patch pad6.patch
(Assignee)

Comment 111

2 years ago
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
Attachment #8658442 - Flags: review?(james)
(Assignee)

Updated

2 years ago
Attachment #8666893 - Attachment is obsolete: true
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.
Attachment #8658442 - Flags: review?(james) → review+

Comment 113

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd0373aafc35
https://hg.mozilla.org/mozilla-central/rev/fd0373aafc35
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Updated

2 years ago
Blocks: 1212427

Updated

2 years ago
Blocks: 1213959
You need to log in before you can comment on or make changes to this bug.