Closed Bug 1242051 Opened 9 years ago Closed 9 years ago

Only install test files as we need to run them

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox48 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: chmanchester, Assigned: chmanchester)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(3 files)

MozReview Request: Bug 1242051 - Extract support files processing from the emitter.
58 bytes, text/x-review-board-request
gps
: review+
Details
MozReview Request: Bug 1242051 - Install test files to the objdir lazily rather than with each invocation of mach.
58 bytes, text/x-review-board-request
gps
: review+
Details
MozReview Request: Bug 1242051 - Add inter-directory test support file dependencies to ini manifests.
58 bytes, text/x-review-board-request
gps
: review+
Details
As another strategy to improve the situation outlined in https://bugzilla.mozilla.org/show_bug.cgi?id=1234728#c0 we can install only test and support files relevant to tests we want to run, instead of syncing the entire objdir from the monolithic install manifest for _tests when invoking the test runner. This is really not trivial, but I have an approach I'll outline after gathering measurements on windows.
The thing that makes this tricky is that a test may depend on an entry in TEST_HARNESS_FILES in a moz.build or a support-files entry in a ini manifest, and that support file might be located far from the test file itself. To deal with this I have a set of patches that splits TEST_HARNESS_FILES and files references in ini test manifests into two separate install manifests under $objdir/_build_manifests/install. * The install manifest for TEST_HARNESS_FILES is installed as part of the build. This is a relatively small number of files (< 1,000). * The install manifest for files in ini test manifests will only be installed before packaging tests. * The mach command for running tests will call a function to generate and execute an InstallManifest based on the result of the test resolver along with the contents of the manifest for TEST_HARNESS_FILES. So changing the mochitest runner to measure test installation time instead of running tests, I get the following (on windows). Before, test installation is about 5s regardless of the number of tests. $ ./mach mochitest Resolver yielded 42781 tests to run Install tests took: 5.45000004768 $ ./mach mochitest dom/ Resolver yielded 7427 tests to run Install tests took: 3.6740000248 $ ./mach mochitest toolkit/mozapps/extensions Resolver yielded 550 tests to run Install tests took: 2.38000011444 $ ./mach mochitest testing/mochitest/tests/Harness_sanity Resolver yielded 25 tests to run Install tests took: 2.21199989319 One thing that's not compatible at all with this plan is ini manifests with only support-files in them. I think these can just be converted to TEST_HARNESS_FILES (currently there are 26 ini manifests with only support-files in them).
Comment #1 sounds like a viable strategy to me. Just using concurrent processes for processing install manifests should speed up wall time significantly [on Windows]. The lazy installation is icing on the cake.
This extracts the logic from the emitter that handles support files in ini manifests to a seperate function in testing.py, so that this logic can be re-used to determine how to install all the files necessary to run a particular test fon the corresponding object in all-tests.json. Review commit: https://reviewboard.mozilla.org/r/32815/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/32815/
This moves test installation for test files out of the monolithic install manifest for $objdir/_tests, and determines the test and support files to install based on the object derived from all-tests.json. Additionally, the files resulting from TEST_HARNESS_FILES are installed, as some tests may depend on them. As a result, the time to install tests when invoking the test runner will scale with the number of tests requested to run rather than the entire set of tests in the tree, resulting in significantly less overhead. Review commit: https://reviewboard.mozilla.org/r/32817/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/32817/
The above is my work in progress. The code for processing test manifests is a little wonky, I'm going to look at sharing more of it with the emitter or add some more tests.
This is getting close: I've verified that $objdir/_tests after running |./mach mochitest| (which resolves all the tests) only differs in insignificant ways before and after these patches. For a artifact-based clobber build this saves about 20s on my Windows laptop.
Comment on attachment 8713781 [details] MozReview Request: Bug 1242051 - Extract support files processing from the emitter. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32815/diff/1-2/
Comment on attachment 8713782 [details] MozReview Request: Bug 1242051 - Install test files to the objdir lazily rather than with each invocation of mach. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32817/diff/1-2/
In order to install test files on-demand, a test must be able to run by installing the support-files specified in its manifest and the contents of TEST_HARNESS_FILES (which are installed wholesale). This is a problem when tests rely on support-files from other directories. This commit moves support files relied on by tests in multiple directories to TEST_HARNESS_FILES so that tests will continue to function correctly when run locally. Review commit: https://reviewboard.mozilla.org/r/34869/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/34869/
Attachment #8719075 - Flags: review?(gps)
Attachment #8713781 - Flags: review?(gps) → review+
Comment on attachment 8713781 [details] MozReview Request: Bug 1242051 - Extract support files processing from the emitter. https://reviewboard.mozilla.org/r/32815/#review31699 ::: python/mozbuild/mozbuild/frontend/emitter.py:1129 (Diff revision 2) > - for pattern in value.split(): > + obj.pattern_installs += patterns Make this `.extend(patterns)`
Comment on attachment 8713782 [details] MozReview Request: Bug 1242051 - Install test files to the objdir lazily rather than with each invocation of mach. https://reviewboard.mozilla.org/r/32817/#review31713 I love the approach! However, I pulled down these commits and `mach xpcshell-test path/to/test/file.js` still installs tens of thousands of files. So it appears something isn't working? Not that it should matter, but I was using an artifact build. ::: Makefile.in:196 (Diff revision 2) > -# For compatibility > +.PHONY: install-tests install-test-files Move the .PHONY for install-test-files to above where the target is first defined. ::: Makefile.in:209 (Diff revision 2) > +# Prepare _tests before any of the other staging/packaging steps. > +# make-stage-dir is a prerequisite to all the stage-* targets in testsuite-targets.mk. > +make-stage-dir: install-test-files This should arguably be in testing/testsuite-targets.mk, as that is where `make-stage-dir` is defined. Historical context: testsuite-targets.mk is included in every Makefile in order to add the make targets for running tests. Once we kill those, testsuite-targets.mk will only contain test packaging logic, at which time we can stop including it in every Makefile. ::: python/mozbuild/mozbuild/controller/building.py:693 (Diff revision 2) > - self._run_make(target='install-tests', append_env=env, pass_thru=True, > + install_test_files(mozpath.normpath(self.topsrcdir), self.topobjdir, I'm 95% sure `self.topsrcdir` is already normalized. ::: python/mozbuild/mozbuild/testing.py:360 (Diff revision 2) > +def install_test_files(topsrcdir, topobjdir, tests_root, test_objs): > + flavor_info = {flavor: (root, prefix, install) for (flavor, root, prefix, install) in TEST_MANIFESTS.values()} > + objdir_dest = mozpath.join(topobjdir, tests_root) > + > + extras = (('head', set()), > + ('tail', set()), > + ('support-files', set()), > + ('generated-files', set())) > + > + patterns, installs, external = [], set(), set() I feel like I've seen this code before. Is there any potential for code reuse? ::: python/mozbuild/mozbuild/testing.py:360 (Diff revision 2) > +def install_test_files(topsrcdir, topobjdir, tests_root, test_objs): > + flavor_info = {flavor: (root, prefix, install) for (flavor, root, prefix, install) in TEST_MANIFESTS.values()} > + objdir_dest = mozpath.join(topobjdir, tests_root) Please add a docstring. ::: python/mozbuild/mozbuild/testing.py:361 (Diff revision 2) > + flavor_info = {flavor: (root, prefix, install) for (flavor, root, prefix, install) in TEST_MANIFESTS.values()} Nit: long line. ::: python/mozbuild/mozbuild/testing.py:400 (Diff revision 2) > + patterns += obj_patterns Use `.extend(obj_patterns)` ::: testing/mochitest/mach_commands.py:476 (Diff revision 2) > + driver.install_tests(tests) `tests` could be an empty list here. Should this fall back to running the `install-test-files` make target like the xpcshell-test command does? Related, perhaps the logic for running that make target should be in ``install_tests()`` so individual mach commands don't have to handle this special case.
Attachment #8713782 - Flags: review?(gps)
Comment on attachment 8719075 [details] MozReview Request: Bug 1242051 - Add inter-directory test support file dependencies to ini manifests. https://reviewboard.mozilla.org/r/34869/#review31727 Hmmm. I'm not super thrilled with this approach because we now have TEST_HARNESS_FILES and support-files doing essentially the same thing with a not very obvious difference in behavior. I'd *really* like to standardize on a single approach or at least clearly delimit differences between TEST_HARNESS_FILES and support-files. I dare say that manifests depending on files installed from another moz.build/manifest file is a bug. If we're going to support running tests by only installing a minimal subset of test files, I think it is reasonable to require moz.build/manifest files to explicitly declare *all* their file dependencies (modulo the test harness itself). This will result in multiple references to the same file. But it is either this or installing all the "support files" all the time or living with non-obvious differences between similar variables. As a short term solution, I don't think installing all support-files all the time is that big of a deal. It's not ideal, but it's still better than installing all the files all the time.
Attachment #8719075 - Flags: review?(gps)
https://reviewboard.mozilla.org/r/32817/#review31713 The xpcshell-test mach command doesn't use the test resolver, so we're still falling back to installing everything in that case. `mach test` and `mach mochitest` should work. > I'm 95% sure `self.topsrcdir` is already normalized. Surprisingy it is not! $ ./mach python -c 'import buildconfig; print(buildconfig.topsrcdir)' c:\Users\cmanchester\m-c
https://reviewboard.mozilla.org/r/34869/#review31727 Installing support-files all the time is problematic because there are a lot. I just double checked, and there are nearly 10,000 of them. I agree the current patch isn't great, I'll need to give this some more thought.
(In reply to Chris Manchester [:chmanchester] from comment #15) > https://reviewboard.mozilla.org/r/32817/#review31713 > > The xpcshell-test mach command doesn't use the test resolver, so we're still > falling back to installing everything in that case. `mach test` and `mach > mochitest` should work. > > > I'm 95% sure `self.topsrcdir` is already normalized. > > Surprisingy it is not! > > $ ./mach python -c 'import buildconfig; print(buildconfig.topsrcdir)' > c:\Users\cmanchester\m-c self.topsrcdir in the MozbuildObject is not necessarily exactly buildconfig.topsrcdir.
(In reply to Mike Hommey [:glandium] from comment #17) > (In reply to Chris Manchester [:chmanchester] from comment #15) > > https://reviewboard.mozilla.org/r/32817/#review31713 > > > > The xpcshell-test mach command doesn't use the test resolver, so we're still > > falling back to installing everything in that case. `mach test` and `mach > > mochitest` should work. > > > > > I'm 95% sure `self.topsrcdir` is already normalized. > > > > Surprisingy it is not! > > > > $ ./mach python -c 'import buildconfig; print(buildconfig.topsrcdir)' > > c:\Users\cmanchester\m-c > > self.topsrcdir in the MozbuildObject is not necessarily exactly > buildconfig.topsrcdir. Ok. I'm finding when running tests locally that they are the same, and neither is normalized.
(In reply to Chris Manchester [:chmanchester] from comment #16) > https://reviewboard.mozilla.org/r/34869/#review31727 > > Installing support-files all the time is problematic because there are a > lot. I just double checked, and there are nearly 10,000 of them. I agree the > current patch isn't great, I'll need to give this some more thought. Ok, I think the way to do this is to leave TEST_HARNESS_FILES alone and add an entry to support-files when a test file depends on a file from another directory. This implies changing the meaning of absolute paths in support-files, because right now they're interpreted as starting at the harness root ($objdir/_tests/testing/mochitest e.g.), but they'll need to be interpreted as starting at $objdir/_tests for files referenced from unrelated test flavors. Tracking down all these and verifying them will be a pain in the neck, but I already found the files in question for this patch, so it's just a matter of going through them.
Greg, can you sign off on the approach in comment 19 before I dive into this?
Flags: needinfo?(gps)
Per IRL chat, we'll want support-files to reference files from other manifests using some special syntax. This is likely referencing the final destination path e.g. "!foo/bar.xml" . moz.build processing can collect references to destination paths during manifest processing. Then once all manifests are processed, verify that all referenced destination paths have a canonical install rule from another manifest.
Flags: needinfo?(gps)
https://reviewboard.mozilla.org/r/32817/#review31713 > I feel like I've seen this code before. Is there any potential for code reuse? I moved this to a class that takes care of memoizing to avoid this duplication.
Comment on attachment 8713781 [details] MozReview Request: Bug 1242051 - Extract support files processing from the emitter. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32815/diff/2-3/
Comment on attachment 8713782 [details] MozReview Request: Bug 1242051 - Install test files to the objdir lazily rather than with each invocation of mach. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32817/diff/2-3/
Comment on attachment 8719075 [details] MozReview Request: Bug 1242051 - Add inter-directory test support file dependencies to ini manifests. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34869/diff/1-2/
Attachment #8719075 - Attachment description: MozReview Request: Bug 1242051 - Move support files used by multiple test directories to TEST_HARNESS_FILES. → MozReview Request: Bug 1242051 - Add a few inter-directory test support file dependencies to prove syntax.
Comment on attachment 8713782 [details] MozReview Request: Bug 1242051 - Install test files to the objdir lazily rather than with each invocation of mach. https://reviewboard.mozilla.org/r/32817/#review36201 I got an error running `mach xpcshell-test services/sync` with this patch. ::: Makefile.in:206 (Diff revision 3) > +install-test-files: NO_REMOVE = 1 > +install-test-files: install-_tests > + $(call py_action,process_install_manifest,--no-remove _tests _build_manifests/install/_test_files) You unconditionally pass `--no-remove`, so why do you have `NO_REMOVE=1`? ::: python/mozbuild/mozbuild/backend/common.py:198 (Diff revision 3) > + if topsrcdir is None: > + topsrcdir = self.topsrcdir > + else: > + topsrcdir = mozpath.normpath(topsrcdir) AFAICT we only call this function from one location. Is there actually a scenario where `obj.topsrcdir` is None? ::: python/mozbuild/mozbuild/backend/common.py:366 (Diff revision 3) > + s = json.dumps({k: v for k, v in self._test_manager.installs_by_path.items() > + if k in self._test_manager.deferred_installs}) You want to throw in a `sort_keys=True` so output is deterministic. ::: testing/testsuite-targets.mk:245 (Diff revision 3) > stage-all: stage-b2g > endif > > -make-stage-dir: > +# Prepare _tests before any of the other staging/packaging steps. > +# make-stage-dir is a prerequisite to all the stage-* targets in testsuite-targets.mk. > +make-stage-dir: install-test-files We're using `NO_REMOVE=1` with the `install-test-files` target. This means that incremental builds in automation could package test files that have been deleted or renamed.
Attachment #8713782 - Flags: review?(gps)
https://reviewboard.mozilla.org/r/32817/#review36201 I still need to test this on OS X and Windows, I'll do so before re-requesting review. > You unconditionally pass `--no-remove`, so why do you have `NO_REMOVE=1`? This was for the install-_tests prerequisite, because generated files would be blown away if we ran that without --no-remove. But install-_tests runs during the build, so it should be necessary here, I'll just ditch the prerequisite. > AFAICT we only call this function from one location. Is there actually a scenario where `obj.topsrcdir` is None? I cargo culted this from def add above, but it doesn't look necessary there either. > We're using `NO_REMOVE=1` with the `install-test-files` target. This means that incremental builds in automation could package test files that have been deleted or renamed. The install-_tests target (with everything from TEST_HARNESS_FILES in it), will be run during the build without --no-remove. Touching a test manifest is enough to trigger this, so I think incremental builds will be ok.
Comment on attachment 8713781 [details] MozReview Request: Bug 1242051 - Extract support files processing from the emitter. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32815/diff/3-4/
Comment on attachment 8713782 [details] MozReview Request: Bug 1242051 - Install test files to the objdir lazily rather than with each invocation of mach. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32817/diff/3-4/
Comment on attachment 8719075 [details] MozReview Request: Bug 1242051 - Add inter-directory test support file dependencies to ini manifests. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34869/diff/2-3/
Comment on attachment 8713782 [details] MozReview Request: Bug 1242051 - Install test files to the objdir lazily rather than with each invocation of mach. https://reviewboard.mozilla.org/r/32817/#review37969 This seems pretty reasonable. This would be much easier to review if it were split into multiple patches. But I suppose it is too late for that. I'm withholding r+ at this time because it is my understanding we need to convert things before this can land. If I am wrong, let me know so I can grant r+. ::: python/mozbuild/mozbuild/testing.py:390 (Diff revision 4) > - else: > + else: > - continue > + continue > - installs.append((full, mozpath.normpath(dest_path))) > + info.installs.append((full, mozpath.normpath(dest_path))) > + return info > + > +def _resolve_installs(paths, topobjdir, manifest): Please add a docstring.
Attachment #8713782 - Flags: review?(gps)
Comment on attachment 8713781 [details] MozReview Request: Bug 1242051 - Extract support files processing from the emitter. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32815/diff/4-5/
Comment on attachment 8713782 [details] MozReview Request: Bug 1242051 - Install test files to the objdir lazily rather than with each invocation of mach. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32817/diff/4-5/
Attachment #8713782 - Flags: review?(gps)
Comment on attachment 8719075 [details] MozReview Request: Bug 1242051 - Add inter-directory test support file dependencies to ini manifests. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34869/diff/3-4/
Attachment #8719075 - Attachment description: MozReview Request: Bug 1242051 - Add a few inter-directory test support file dependencies to prove syntax. → MozReview Request: Bug 1242051 - Add inter-directory test support file dependencies to ini manifests.
Attachment #8719075 - Flags: review?(gps)
Comment on attachment 8719075 [details] MozReview Request: Bug 1242051 - Add inter-directory test support file dependencies to ini manifests. https://reviewboard.mozilla.org/r/34869/#review39043 ::: accessible/tests/mochitest/pivot/a11y.ini:5 (Diff revision 4) > + !/accessible/tests/mochitest/browser.js > + !/accessible/tests/mochitest/common.js > + !/accessible/tests/mochitest/events.js > + !/accessible/tests/mochitest/layout.js > + !/accessible/tests/mochitest/pivot.js > + !/accessible/tests/mochitest/role.js > + !/accessible/tests/mochitest/states.js > + !/accessible/tests/mochitest/text.js I keep seeing the same list of files. This feels like a DRY violation and will lead to a lot of overhead maintaining when someone e.g. adds a new .js that is included by a commonly included .js file already in this list. Can we not use wildcards? ::: devtools/client/inspector/markup/test/browser.ini:43 (Diff revision 4) > + !/devtools/client/commandline/test/helpers.js > + !/devtools/client/inspector/test/head.js > + !/devtools/client/framework/test/shared-head.js > + !/devtools/client/shared/test/test-actor.js > + !/devtools/client/shared/test/test-actor-registry.js More excessive repetition. Although this one is harder to wildcard :/ ::: services/fxaccounts/tests/xpcshell/xpcshell.ini:5 (Diff revision 4) > +support-files = > + !/services/common/tests/unit/head_helpers.js > + !/services/common/tests/unit/head_http.js These files are listed in the `[head]` section. Should we automatically add `[head]` and `[tail]` to support-files?
Attachment #8719075 - Flags: review?(gps)
https://reviewboard.mozilla.org/r/34869/#review39043 > I keep seeing the same list of files. This feels like a DRY violation and will lead to a lot of overhead maintaining when someone e.g. adds a new .js that is included by a commonly included .js file already in this list. > > Can we not use wildcards? The wildcard would be accessible/tests/mochitest/*.js, which is a little general, but ok. > These files are listed in the `[head]` section. Should we automatically add `[head]` and `[tail]` to support-files? This only happens in a few places, I'm not sure it's worth it.
Comment on attachment 8713781 [details] MozReview Request: Bug 1242051 - Extract support files processing from the emitter. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32815/diff/5-6/
Attachment #8719075 - Flags: review?(gps)
Comment on attachment 8713782 [details] MozReview Request: Bug 1242051 - Install test files to the objdir lazily rather than with each invocation of mach. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32817/diff/5-6/
Comment on attachment 8719075 [details] MozReview Request: Bug 1242051 - Add inter-directory test support file dependencies to ini manifests. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34869/diff/4-5/
https://reviewboard.mozilla.org/r/34869/#review39043 > More excessive repetition. Although this one is harder to wildcard :/ So, I'm not exactly thrilled with how this turns out, either. It's an easy enough idea, but the sheer volume of annotations necessary makes it pretty unwieldy. The prior approach (moving support files used by multiple directories to TEST_HARNESS_FILES, comment 11) avoided this repetition. We could consider going back to that.
Assignee: nobody → cmanchester
Comment on attachment 8713782 [details] MozReview Request: Bug 1242051 - Install test files to the objdir lazily rather than with each invocation of mach. https://reviewboard.mozilla.org/r/32817/#review39287 This is a complicated change. But it looks good AFAICT. Please try to land this early in a week when you'll be around to triage fallout. ::: python/mozbuild/mozbuild/test/frontend/test_emitter.py:453 (Diff revision 6) > + self.assertEqual(len(supported.installs), 3) > + self.assertEqual(len(supported.deferred_installs), 3) > + self.assertEqual(len(child.installs), 3) > + self.assertEqual(len(child.pattern_installs), 1) I'd feel much better about this test if it verified paths instead of merely counts.
Attachment #8713782 - Flags: review?(gps) → review+
Comment on attachment 8719075 [details] MozReview Request: Bug 1242051 - Add inter-directory test support file dependencies to ini manifests. https://reviewboard.mozilla.org/r/34869/#review39301 rs=me
Attachment #8719075 - Flags: review?(gps) → review+
https://reviewboard.mozilla.org/r/32817/#review39305 Could I also get you to update the in-tree docs to include a blurb about test manifests needing to declare dependencies in other directories? You'll also want to send an email out to firefox-dev and dev-platform when this lands to let people know they need to start explicitly capturing dependencies.
Autophone appears to handle the changes well. Ran a local test on a variety of mochitest/reftest unit tests and found no issues: https://treeherder.allizom.org/#/jobs?repo=try&revision=eecb1e94d4ab&filter-searchStr=autophone&exclusion_profile=false&filter-tier=1&filter-tier=2&filter-tier=3&group_state=expanded
With 544f0666205a revision, accessibility tests are not running locally because there's an erroneous line in accessible/tests/mochitest/selectable/a11y.ini. Would anyone know if they even run on our CI?
Flags: needinfo?(cmanchester)
Sorry about the bustage. These tests run in CI as a part of the "oth" symbol on treeherder, but a different facility is used to install tests there, so we don't see this. I'll land a fix shortly.
Flags: needinfo?(cmanchester)
Depends on: 1262626
Depends on: 1278998
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: