1134395 - [manifestparser] test relpaths not calculated properly with the mozbuild model (no root manifest)

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Andrew Halberstadt [:ahal]]

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 [1], 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) [2]. 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.

[1] https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/backend/common.py#168
[2] https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestparser/manifestparser/manifestparser.py?from=manifestparser.py#228

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: 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

Comment 2

[Andrew Halberstadt [:ahal]]

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

Comment 3

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/4275/#review3633

Touching this code always scares me. But this change looks good to me.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8568815 [details]
MozReview Request: bz://1134395/ahal

https://reviewboard.mozilla.org/r/4273/#review3635

Ship It!

Comment 5

[Andrew Halberstadt [:ahal]]

(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

Comment 6

[Andrew Halberstadt [:ahal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8250f4a84e81

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8250f4a84e81

Comment 9

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: Bug 1134395 - mozbuild should pass in rootdir to manifestparser to properly calculate test relpaths, r=gps