Closed
Bug 1283919
Opened 8 years ago
Closed 8 years ago
Improve test package archiver for collecting files from directories referenced by a root manifest
Categories
(Firefox Build System :: General, defect)
Tracking
(firefox51 fixed)
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
Attachments
(1 file, 1 obsolete file)
Right now we still have to use make targets for test package archiving when tests are spread around in different components: https://dxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.mk#334 I would like to improve the test package archiver so it can collect all the referenced test files from listed manifests. With that we could drastically reduce the make files, and possibly allow to run more tests from the src tree.
Assignee | ||
Updated•8 years ago
|
Summary: Improve test package archiver for test files collection → Improve test package archiver for collecting test files from different directories
Assignee | ||
Comment 1•8 years ago
|
||
So that doesn't seem to be too complicated. It's just work which would need a bit of time. I will get started on this now. I had a first look at the make steps which are currently used to collect the marionette tests. These are the following: 1. Calling print-manifest-dirs.py with the top-level manifest file 2. Creating a tar archive from all folders under topsrcdir as listed in the top-level manifest file 3. Extracting the tar archive to obj/dist/test-stage 4. test_archive.py picks those up to include in common.tests.zip The improvement we want to have is: 1. Call test_archive.py which collects the tests and directly includes them in the target tests.zip file With that change we save a full tar/untar cycle.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
I got the print_manifests.py file integrated into test_archive.py. For testing I used the Marionette files as example and all is working perfectly. With it I also have removed the make file instructions in testing/testsuite-targets.mk. I will massage the patch tomorrow and will upload a first WIP.
Assignee | ||
Updated•8 years ago
|
Summary: Improve test package archiver for collecting test files from different directories → Improve test package archiver for collecting files from directories referenced by a root manifest
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8782006 [details] Bug 1283919 - Improve test package archiver for collecting files from directories referenced by a root manifest Gregory, I would love to get some feedback for that patch before I start checking what kind of unit tests I could implement. Please let me know if it looks fine, or if things are wrongly implemented.
Attachment #8782006 -
Flags: feedback?(gps)
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8782006 [details] Bug 1283919 - Improve test package archiver for collecting files from directories referenced by a root manifest https://reviewboard.mozilla.org/r/72298/#review70690 This looks mostly good! ::: python/mozbuild/mozbuild/action/test_archive.py:437 (Diff revision 2) > + '**/docs', > + '**/*.log', The addition of these 2 scares me a little. I could see someone getting really confused as to why their directory or .log files aren't getting packaged in automation. ::: python/mozbuild/mozbuild/action/test_archive.py:465 (Diff revision 2) > + dirs.add(os.path.dirname(manifest)) > + > + # output the directories of all the other manifests > + test_manifest = TestManifest() > + test_manifest.read(os.path.join(topsrcdir, manifest)) > + for i in test_manifest.get(): > + d = os.path.dirname(i['manifest'])[len(topsrcdir) + 1:] > + dirs.add(d) It seems you don't need the `topsrcdir` argument at all: you can just derive it from `os.path.dirname(manifest)`. ::: python/mozbuild/mozbuild/action/test_archive.py:474 (Diff revision 2) > + test_manifest.read(os.path.join(topsrcdir, manifest)) > + for i in test_manifest.get(): > + d = os.path.dirname(i['manifest'])[len(topsrcdir) + 1:] > + dirs.add(d) > + > + dirs = ['{}/**'.format(p.replace('\\', '/')) for p in dirs] We have the `mozpath` module that provides path manipulation functions that always use Unix style paths. You should use that throughout this function so you don't have to normalize. The adding of the wildcard could also be added into the for loop above.
Updated•8 years ago
|
Attachment #8782006 -
Flags: feedback?(gps) → feedback+
Assignee | ||
Comment 9•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8782006 [details] Bug 1283919 - Improve test package archiver for collecting files from directories referenced by a root manifest https://reviewboard.mozilla.org/r/72298/#review70690 > The addition of these 2 scares me a little. I could see someone getting really confused as to why their directory or .log files aren't getting packaged in automation. Ok, I will get rid of both to keep parity with the current packaging. > It seems you don't need the `topsrcdir` argument at all: you can just derive it from `os.path.dirname(manifest)`. If you don't use topsrcdir here, the command "mach build package-tests" will fail if called from the obj dir. Given that it was supported before I didn't want to break it. Maybe I should add a comment here explaining it. Does it make sense to you? > We have the `mozpath` module that provides path manipulation functions that always use Unix style paths. You should use that throughout this function so you don't have to normalize. > > The adding of the wildcard could also be added into the for loop above. Wasn't aware of that helpful module. My next commit will fix that.
Assignee | ||
Comment 10•8 years ago
|
||
I did some more work on this bug and was able to combine manifests with reftest manifests, so that we do no longer have a special case for the reftests. We can now define the test entry as any other one. After some cleanup and testing I will upload a new version of the patch.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8782007 -
Flags: review?(gps)
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8782006 [details] Bug 1283919 - Improve test package archiver for collecting files from directories referenced by a root manifest Gregory, due to a bug in mozreview I cannot request review from you for the first commit. Lets do it this way and I hope it will work.
Attachment #8782006 -
Flags: review?(gps)
Assignee | ||
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8782006 [details] Bug 1283919 - Improve test package archiver for collecting files from directories referenced by a root manifest https://reviewboard.mozilla.org/r/72298/#review72010 ::: python/mozbuild/mozbuild/action/test_archive.py:477 (Diff revision 3) > dirs = set() > + > for p in manifests: > + p = os.path.join(topsrcdir, p) > + > + if p.endswith('.ini'): Maybe not that ideal by checking for the extension, but still better as having reftests handled separately. Or is there an existing way to check for a manifest type? I haven't found one, and don't want to necessarily touch both Manifest classes.
Assignee | ||
Comment 15•8 years ago
|
||
Gregory, will you have the time to review this patch or shall I ask someone else? Thanks.
Flags: needinfo?(gps)
Comment 17•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8782006 [details] Bug 1283919 - Improve test package archiver for collecting files from directories referenced by a root manifest https://reviewboard.mozilla.org/r/72298/#review72010 > Maybe not that ideal by checking for the extension, but still better as having reftests handled separately. Or is there an existing way to check for a manifest type? I haven't found one, and don't want to necessarily touch both Manifest classes. There isn't a better way. Sniffing the file extension feels OK.
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8782006 [details] Bug 1283919 - Improve test package archiver for collecting files from directories referenced by a root manifest https://reviewboard.mozilla.org/r/72298/#review73314 This commit changes a lot of things. In the future, please consider splitting your changes into smaller parts to make review easier.
Attachment #8782006 -
Flags: review?(gps) → review+
Comment 19•8 years ago
|
||
mozreview-review |
Comment on attachment 8782007 [details] Bug 1283919 - Move packaging of Marionette from make to the test archiver https://reviewboard.mozilla.org/r/72300/#review73318 This looks good. Does this commit also remove the only users of the `MARIONETTE_*` moz.build variables? If so, there should be a follow-up to nuke those variables from moz.build. There are references in at least python/mozbuild/mozbuild/frontend/context.py and python/mozbuild/mozbuild/testing.py.
Attachment #8782007 -
Flags: review?(gps) → review+
Assignee | ||
Comment 20•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8782006 [details] Bug 1283919 - Improve test package archiver for collecting files from directories referenced by a root manifest https://reviewboard.mozilla.org/r/72298/#review73314 I know, but I wanted to make sure that I do not add or change code in a commit which is not working as expected. Splitting up all changes at the end is even harder for me locally. I will see what I can do the next time. Thanks.
Assignee | ||
Comment 21•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8782007 [details] Bug 1283919 - Move packaging of Marionette from make to the test archiver https://reviewboard.mozilla.org/r/72300/#review73318 Oh! I totally missed, that there are references too. I will check this tomorrow but yes, nothing should remain in tree anymore.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8782007 [details] Bug 1283919 - Move packaging of Marionette from make to the test archiver https://reviewboard.mozilla.org/r/72300/#review73318 I got those definitions removed with the latest commit. So Marionette is completely gone from moz.build now.
Comment 25•8 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7e1f25f59298 Improve test package archiver for collecting files from directories referenced by a root manifest r=gps https://hg.mozilla.org/integration/autoland/rev/a328778db08d Move packaging of Marionette from make to the test archiver r=gps
Comment 26•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7e1f25f59298 https://hg.mozilla.org/mozilla-central/rev/a328778db08d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee | ||
Comment 27•8 years ago
|
||
As we figured out yesterday the removal of the moz.build entries of Marionette was a mistake which needs to be reverted. Ultimately this is the way how we want to collect tests. We should get this backed out.
Comment 28•8 years ago
|
||
backout |
https://hg.mozilla.org/mozilla-central/rev/ab70808cd4b6
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla51 → ---
Assignee | ||
Comment 29•8 years ago
|
||
The faulty part of this patch is actually the second commit. It's about Marionette and actually does not really rely into this bug. I will go ahead and strip of the second commit, and move it into a separate bug which will be for the Marionette changes. With that we can safely re-land the commit for the general test_archiver changes.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8782007 -
Attachment is obsolete: true
Comment 31•8 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3d1777fa2c34 Improve test package archiver for collecting files from directories referenced by a root manifest r=gps
Comment 32•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3d1777fa2c34
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•