Mochitest should only parse test manifests once

RESOLVED FIXED in Firefox 39

Status

Testing
Mochitest
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ahal, Assigned: ahal)

Tracking

unspecified
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Currently mochitest parses the test manifests at least three times (and once per directory in the event of --run-by-dir). This is both inefficient and can cause obscure errors where the list of tests differs slightly from one call to another.

Instead the manifests should just be parsed once and the resulting list of tests saved for the future.
(Assignee)

Comment 1

3 years ago
The function that does the parsing is 'getActiveTests':
https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=getActiveTests&redirect=true

This function gets called by 'getTestsToRun', 'getDirectories' and 'buildTestPath' which themselves can get called multiple times. Each time it is invoked a whole new manifest object is instantiated.
(Assignee)

Comment 2

3 years ago
This would also fix a bug I found.

When using --manifest with --run-by-dir, mochitest will run the incorrect set of tests. At first getActiveTests() uses options.manifestFile correctly, and returns the correct set of tests. But in buildTestPath (as well as initializeLooping) we change options.manifestFile to something else [1]. Normally this is fine since we've already called getActiveTests(). But with --run-by-dir enabled, getActiveTests() is called once per directory [2]. Now options.manifestFile no longer points to the original manifest we passed in and we'll fall back to using the master manifest [3] instead (completely ignoring --manifest).

In automation we don't use --manifest, so this isn't a problem. But we use it with mach [4]. Unfortunately there's no failure, just the wrong set of tests subtly getting run.


[1] https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#776
[2] https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#2058
[3] https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#2501
[4] https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/mach_commands.py#432
(Assignee)

Updated

3 years ago
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
Created attachment 8579327 [details]
MozReview Request: bz://1144194/ahal

/r/5597 - Bug 1144194 - Only parse test manifests once in mochitest, r=jmaher

Pull down this commit:

hg pull review -r 3353402fa0bfa27afbbfad748ead180735605158
(Assignee)

Comment 4

3 years ago
Comment on attachment 8579327 [details]
MozReview Request: bz://1144194/ahal

/r/5597 - Bug 1144194 - Only parse test manifests once in mochitest, r=jmaher

Pull down this commit:

hg pull review -r 3353402fa0bfa27afbbfad748ead180735605158
Attachment #8579327 - Flags: review?(jmaher)
Comment on attachment 8579327 [details]
MozReview Request: bz://1144194/ahal

https://reviewboard.mozilla.org/r/5595/#review4623

Ship It!
Attachment #8579327 - Flags: review?(jmaher) → review+
(Assignee)

Comment 6

3 years ago
Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2c7d5841f7f

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0918170249a
https://hg.mozilla.org/mozilla-central/rev/e0918170249a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Assignee)

Comment 8

3 years ago
Comment on attachment 8579327 [details]
MozReview Request: bz://1144194/ahal
Attachment #8579327 - Attachment is obsolete: true
Attachment #8619783 - Flags: review+
(Assignee)

Comment 9

3 years ago
Created attachment 8619783 [details]
MozReview Request: Bug 1144194 - Only parse test manifests once in mochitest, r=jmaher
You need to log in before you can comment on or make changes to this bug.