1293259 - Ability to resolve tests outside of a build context (i.e srcdir or test package)

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[Andrew Halberstadt [:ahal]]

The root problem I'm trying to solve here is that it's not easy to run a variant of mochitest other than 'plain' from an interactive loaner. To keep the same workflow we do in mozilla-central, we need to enable test resolving outside of a build context. This should help both with running from a test package on an interactive loaner, and also be a stepping stone towards running tests from the srcdir.

The easy path here is to copy 'all-tests.json' (and TestResolver) to the tests.zip and tweak TestResolver to accept paths other than the objdir. This will solve the interactive loaner case, but still depends on the build to generate 'all-tests.json'.

My preferred solution is to either write a new test resolver in mozbase or move the existing one out of mozbuild and adapt it into mozbase. This new mozbase resolver wouldn't depend on a build object in any way. This would move us much further along to running tests from the source. Also keep in mind the build system could still hook into this mozbase resolver if it needed to.

That being said I think I need to understand why the build is processing tests/test manifests in the first place. Is it a performance enhancement? Does the build system do anything else with that data besides writing out 'all-tests.json'? Is processing test manifests at test runtime too expensive (i.e can manifestparser simply be our resolver)?

Gps/chmanchester/ted, I'd love to hear your thoughts on your preferred paths forward here.

Comment 1

[Andrew Halberstadt [:ahal]]

Please feel free to disregard the needinfo if you don't have any opinions here.

Comment 2

[(not currently active) Ted Mielczarek]

The build system reads the test manifests so it can do test installation to the objdir. After chmanchester's "just-in-time test installation" work I'm not sure if that's super necessary.

We do generate top-level manifests for many of our harnesses from the set of manifests listed in moz.build, right? Each of those should contain the full list of tests for that harness for that build.

Comment 3

[Chris Manchester (limited bugmail, email directly)]

I don't really have a preference for which direction we head here, although I imagine we want to keep an eye towards running tests from a source checkout some day (with some of the ideas I've heard from gps recently this is sounding closer than I would have thought).

I'll keep an eye on this though, because processing test manifests and writing out all-tests.json is pretty slow, and we would very much like to make it faster.

Comment 4

[Gregory Szorc [:gps]]

I don't really have a preference either. I too would like to focus on running tests in automation from a source checkout with a minimal build context - kinda like we do with artifact builds. Then we can have the same code paths in automation that local developers use. It will be sweet.

Comment 5

[Andrew Halberstadt [:ahal]]

I've been thinking about this a bit and think I have a good idea to proceed. I'll get a prototype "mozresolve" module going in mozbase. This will have an abstract base resolver which will be implemented by various concrete resolvers. E.g:

* BuildObjResolver - Resolves paths using the build system. This will hold the current behaviour, and will act as a legacy stop-gap until we transition things off the build system, though I'm sure many paths will continue to be resolved this way.

* FileSystemResolver - Resolves paths using the file system alone (i.e path prefixes and/or globbing). This will likely lean on mozpath a bunch.

* ManifestResolver - Resolves paths using manifestparser manifests. May also handle reftest manifests? This would mostly only be applicable to test paths.

With these three implementations, we can swap them in and out as needed. E.g, as we start moving test harnesses to only require the srcdir, we can switch from BuildObjResolver to ManifestResolver (or FileSystemResolver), with hopefully no hiccups. I don't really care if this lives in mozbase, or its own module under /python. I'll try to get a prototype going first and we can bikeshed about stuff like that later.

Off the top of my head (and apart from test harnesses/build system), there are already a bunch of things that do their own path resolving that could use this:
- ./mach python-tests --path-only
- ./mach lint
- interactive workers

Comment 6

[(not currently active) Ted Mielczarek]

If we're running tests from a source checkout we'll still have a build system context, it just won't be a fully built one--more like an artifact build. I think we could still resolve tests from the build system there, we just might need to make sure that it's performant.

Comment 7

[Andrew Halberstadt [:ahal]]

We could yes, but why redo the work that manifestparser already does?

The input to the build system resolver is all-tests.json. The input to the manifest resolver is a root manifest + mozinfo.json. Both all-tests.json and mozinfo.json are generated by the build, but the latter has some advantages:

1. Consistent with automation
2. Easier to download/access (can work both from an objdir and tests.zip)
3. Simpler (build system doesn't need to know anything about tests)

Manifestparser even has a built-in pathprefix filter, so we wouldn't be losing the ability to pass in a directory and run all tests under that.

I confess I have a second use case for mozresolve that is not even test related. There is some pretty hairy resolving logic in mozlint that I want to share with taskcluster configs, I thought mozresolve could be a good place to stash that as well.

Comment 8

[Andrew Halberstadt [:ahal]]

Apologies for the large braindump, this is as much for my benefit as anyone else's. I hope to clarify what I see as the problem here and what a potential solution might look like. I'm still not sure if this is something worth pursuing or not, it's definitely a lot more complex than I initially thought.

So first I'd like to make a few observations:
A) The build system processes .ini manifests and generates a file called "all-tests.json"
B) The build system also generates root .ini manifests for each test suite
C) Only mach commands use "all-tests.json" to resolve tests
D) In CI, the automation re-parses the generated root .ini manifests with manifestparser

The build system also deals with reftest, wpt and python unittests that don't use manifestparser manifests. I'm ignoring these for now just for simplicity, but we'll need to take these into consideration when deciding what to do.


In my mind, there are two problems with the above scenario:

1) The build system spends a non-trivial amount of time processing manifests (A). Under the assumption that reading "all-tests.json" is faster than parsing with manifestparser (I'm not sure if this is true or not), then this build system processing would be a nice little win. We'd be doing it in one place instead of every single test job. The problem here is that the test jobs then go and re-parse everything with manifestparser anyway (D), so this potential win is not realized.

2) There are two separate code paths for resolving tests (A and B). This could potentially result in divergent behaviour when running with mach vs in CI (e.g pushing to try with paths, using one-click loaners etc). It also makes things more confusing, it took me a fair bit of time to get a grasp on how test resolving worked in varying contexts.


I think there are two paths forward here (aside from "do nothing" which still remains a very valid path):

I) Stop generating "all-tests.json" and refactor mozbuild.testing.TestManifest to use manifestparser directly for resolving tests. The build system would continue to generate root .ini manifests which would be an input to TestManifest.

Pros:
* Build system no longer needs to parse test manifests -> faster
* Same code path for resolving between mach/CI

Cons:
* Less powerful, build system knows less about tests may limit what we can do
* Test resolving may be slower when using mach (no perf change for test jobs in CI)

II) Refactor mozbuild.testing into a more standalone module and get CI to use that instead of re-parsing the root test manifests. Put "all-tests.json" and TestResolver in the test package.

Pros:
* Potential perf improvement for all test jobs (if "all-tests.json" resolving is faster than manifestparser)
* Maintain test data in the build system

Cons:
* Still have two systems for resolving tests
* Less flexible, still tightly coupled to the build system


My preference between the two is (I). It feels like a cleaner solution and seems to align better in a world where test harnesses can be run from the srcdir. I actually have a Proof of Concept patch nearly completed, though it won't be in a landable state anytime soon due to reftest/python/wpt. I'll try to get it cleaned up and posted for feedback by the end of the month.

And to re-iterate, I'm still not entirely sure the best path forward here. I hope this post at least provides a glimpse into my current thought process. I'd love to hear any feedback on all this.

Comment 9

[Chris Manchester (limited bugmail, email directly)]

I've been looking into build-backend perf recently, which is where we generate all-tests.json. I found from my measurements that parsing the test manifests themselves is not too bad (less than 400ms on linux, while the reader alone takes ~7 seconds), but writing out the tests database is slow, and will still take a few seconds after bug 1312574.

So from the build system's perspective, getting rid of all-tests.json would certainly be a win. |./mach test| and |./mach try| use it for some pretty interesting queries, so it might take a lot to replace this functionality. As you found it does the job of finding a common ground between manifest formats.

Comment 10

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1293259 - [manifestparser] Add ability to match wildcards in "pathprefix" filter

This is a WIP. I will eventually split it out into smaller more manageable patches, but
for now it is useful to keep it as one. It has only been tested with |mach mochitest| and
even then there are known reasons for tests to fail. It also needs to be tested to make
sure the same set of tests are being resolved, as well as profiled to measure any performance
wins.

This change attempts to speed up the build system by not processing test manifest files
at build time. You'll notice large swathes of build backend/frontend code deleted.  This
means no "all-tests.pkl" files. Instead, the manifest.ini trees will be walked and processed
at test runtime. This probably takes no longer than reading "all-tests.pkl" did, but
I haven't profiled this yet. In CI, the manifest trees were being walked at runtime anyway,
so there should be no perf change there.

This patch has only been tested with |mach mochitest|. Processing of reftest, wpt and
python unittests has been removed for now. We'll either need to create manifest based
resolvers for each of those, or continue using the "all-tests.pkl" method. My preference
is for the former.

Another problem with this approach is that the concept of "deferred_installs" is no
longer viable. Because we aren't processing all the manifests at build time, we have
no way of finding the original install belonging to a deferred_install. This patch instead
copies deferred_installs from the path in topsrcdir. This works for most cases, but in
some cases this will result in the deferred_install being copied to the wrong destination
which means mochitests will fail due to missing support-files. We'll either need to update
all the manifests, or figure out something more clever to handle this case.

Review commit: https://reviewboard.mozilla.org/r/90772/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90772/

Comment 11

[Andrew Halberstadt [:ahal]]

Bleh, I was hoping to use my commit message as bug comment, but with the mozreview comments hidden by default that doesn't get the visibility I was hoping for.. Copy/pasting commit message here.

This is a WIP. I will eventually split it out into smaller more manageable patches, but
for now it is useful to keep it as one. It has only been tested with |mach mochitest| and
even then there are known reasons for tests to fail. It also needs to be tested to make
sure the same set of tests are being resolved, as well as profiled to measure any performance
wins.

This change attempts to speed up the build system by not processing test manifest files
at build time. You'll notice large swathes of build backend/frontend code deleted.  This
means no "all-tests.pkl" files. Instead, the manifest.ini trees will be walked and processed
at test runtime. This probably takes no longer than reading "all-tests.pkl" did, but
I haven't profiled this yet. In CI, the manifest trees were being walked at runtime anyway,
so there should be no perf change there.

This patch has only been tested with |mach mochitest|. Processing of reftest, wpt and
python unittests has been removed for now. We'll either need to create manifest based
resolvers for each of those, or continue using the "all-tests.pkl" method. My preference
is for the former.

Another problem with this approach is that the concept of "deferred_installs" is no
longer viable. Because we aren't processing all the manifests at build time, we have
no way of finding the original install belonging to a deferred_install. This patch instead
copies deferred_installs from the path in topsrcdir. This works for most cases, but in
some cases this will result in the deferred_install being copied to the wrong destination
which means mochitests will fail due to missing support-files. We'll either need to update
all the manifests, or figure out something more clever to handle this case.

Comment 12

[Andrew Halberstadt [:ahal]]

Feel free to take a look and provide feedback if you want. Though there are still many rough edges to iron out, I think this patch proves that a manifestparser based approach to test resolving is viable. Build time improvements seem to be a modest ~6 seconds on my local machine.

I will clean it up a bit next week, though I'm not sure if I'll be driving this to completion or not. The main work items are reftest/wpt/python and dealing with deferred_installs better (or updating the manifests to not use them).

Comment 13

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1293259 - [manifestparser] Use rootdir when searching for child manifests

Review commit: https://reviewboard.mozilla.org/r/93156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93156/

Comment 14

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1293259 - Write list of master test manifests to a json file in the objdir

Review commit: https://reviewboard.mozilla.org/r/93158/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93158/

Comment 15

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1293259 - Stop reading/processing test manifests as part of the build

This is a WIP. It has only been tested with |mach mochitest| and even then there are known
reasons for tests to fail. It also needs to be tested to make sure the same set of tests
are being resolved. It seems to save a modest ~6 seconds off build times.

This change attempts to speed up the build system by not processing test manifest files
at build time. You'll notice large swathes of build backend/frontend code deleted.  This
means no "all-tests.pkl" files. Instead, the manifest.ini trees will be walked and processed
at test runtime.

This patch has only been tested with |mach mochitest|. Processing of reftest, wpt and
python unittests has been removed for now. We'll either need to create manifest based
resolvers for each of those, or continue using the "all-tests.pkl" method. My preference
is for the former.

Another problem with this approach is that the concept of "deferred_installs" is no
longer viable. Because we aren't processing all the manifests at build time, we have
no way of finding the original install belonging to a deferred_install. This patch instead
copies deferred_installs from the path in topsrcdir. This works for most cases, but in
some cases this will result in the deferred_install being copied to the wrong destination
which means mochitests will fail due to missing support-files. We'll either need to update
all the manifests, or figure out something more clever to handle this case.

Review commit: https://reviewboard.mozilla.org/r/93160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/93160/

Comment 16

[Andrew Halberstadt [:ahal]]

Comment on attachment 8807694 [details]
Bug 1293259 - [manifestparser] Add ability to match wildcards in "pathprefix" filter

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90772/diff/1-2/

Comment 17

[Andrew Halberstadt [:ahal]]

So this is actually going to end up blocking running python unittests in their own tasks that don't depend on a build (after bug 1317970 lands).. which is how I was planning to get them out of 'make check'.

So I'm going to get a patch series up that handles both methods of resolving (all-tests vs manifestparser). This will also have the benefit of allowing us to ignore reftest/wpt for the time being and get the work here landed. Initially I'll probably just deal with python unittests and file a follow-up for the other harnesses.

Comment 18

[Andrew Halberstadt [:ahal]]

The TestResolver has long been outside of the build system. And recently grew the ability to resolve all tests without a build context by scanning the filesystem for moz.build files.