Closed Bug 1134395 Opened 5 years ago Closed 5 years ago
[manifestparser] test relpaths not calculated properly with the mozbuild model (no root manifest)
MozReview Request: Bug 1134395 - mozbuild should pass in rootdir to manifestparser to properly calculate test relpaths, r=gps
39 bytes, text/x-review-board-request
This was a fun one to debug. First some back story. In bug 1131098 I'm trying to move chunking algorithms out of mochitest and into manifestparser. One of those algorithms (chunk_by_dir) requires the relative path of each test to the source of the repo. I noticed that each test object's 'relpath' attribute was simply the file name of the test. I also noticed some extra non-standard attributes like 'file_relpath' and 'dir_relpath' (these extra attributes were added by mozbuild , likely because the standard 'relpath' attribute wasn't doing the right thing). Now 'file_relpath' was exactly what I needed, but unfortunately I couldn't use it because it is non-standard and the chunking algorithm lives in manifestparser core. So I did some more investigation. The manifestparser docs state that 'relpath' is the "relative path starting from the root manifest location". This is accomplished by setting manifest.rootdir to the directory of the first manifest file passed in (aka the "root" manifest) . But in the case of mochitests (and others) there is no root manifest. Instead the build system is creating a new TestManifest object for each manifest in the tree. The rootdir is therefore set to the directory of each test manifest in turn, and relpath is calculated as just the test's file name (as more often than note, tests live in the same directory as their associated manifests). The solution is to tweak the definition of relpath a little. In the case of mochitest/xpcshell/etc 'relpath' should be the path relative to the root of the repository (gecko), since there is no root manifest. This can be accomplished by passing the root repository path in to TestManifest.__init__(). This sounds confusing and complicated, but the patch is quite simple. Will upload in a bit after some more testing.  https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/backend/common.py#168  https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestparser/manifestparser/manifestparser.py?from=manifestparser.py#228
/r/4275 - Bug 1134395 - mozbuild should pass in rootdir to manifestparser to properly calculate test relpaths, r=gps Pull down this commit: hg pull review -r 9853847b7c79b9360f5d5870d7359e7c9e362f94
Comment on attachment 8568815 [details] MozReview Request: bz://1134395/ahal /r/4275 - Bug 1134395 - mozbuild should pass in rootdir to manifestparser to properly calculate test relpaths, r=gps Pull down this commit: hg pull review -r 9853847b7c79b9360f5d5870d7359e7c9e362f94
Attachment #8568815 - Flags: review?(gps)
https://reviewboard.mozilla.org/r/4275/#review3633 Touching this code always scares me. But this change looks good to me.
Comment on attachment 8568815 [details] MozReview Request: bz://1134395/ahal https://reviewboard.mozilla.org/r/4273/#review3635 Ship It!
Attachment #8568815 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #3) > Touching this code always scares me. But this change looks good to me. I spent a fair amount of time smoothing over some issues I initially caused. Here are some passing try runs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=001da02ebe73 https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ed201d01fa3
You need to log in before you can comment on or make changes to this bug.