"mach test toolkit/components/extensions/test/xpcshell/*.js" skips many tests, but "mach xpcshell-test" works
Categories
(Testing :: XPCShell Harness, defect, P2)
Tracking
(firefox-esr60 fixed, firefox65 wontfix, firefox66 fixed, firefox67 fixed)
People
(Reporter: robwu, Assigned: robwu)
References
Details
(Keywords: regression)
Attachments
(1 file)
Assignee | ||
Comment 1•6 years ago
|
||
Comment 2•6 years ago
|
||
Updated•6 years ago
|
Updated•6 years ago
|
Comment 3•6 years ago
|
||
Comment 4•6 years ago
|
||
Comment 5•6 years ago
|
||
Comment 6•6 years ago
|
||
Sorry, haven't been able to find time for this. There is something very interesting happening, possibly with far-reaching consequences.
Assignee | ||
Comment 7•6 years ago
|
||
I think that the culprit is here:
testing/mozbase/manifestparser/manifestparser/manifestparser.py:153
def _read(self, root, filename, defaults, defaults_only=False, parentmanifest=None):
...
self.manifest_defaults[filename] = defaults
...
# TODO: keep track of included file structure:
# self.manifests = {'manifest.ini': 'relative/path.ini'}
if section.startswith('include:'):
include_file = read_file('include:')
if include_file:
include_defaults = data.copy()
self._read(root, include_file, include_defaults, parentmanifest=filename)
When a manifest is included, its default are saved in the manifest_defaults
member, keyed by the name of that included manifest. This has the risk of collisions, such as when a manifest is included multiple times, inheriting different sets of variables (this bug, for example).
I suppose that this could be fixed by including the parent manifest in the key (instead of just the filename
). If stored in manifest_defaults
, then the manifests
method needs to also account for these changes (since it is supposed to return paths to manifests).
Lastly, in order to pick up the changes, the new key needs to be appended to the default_manifests
list at https://searchfox.org/mozilla-central/rev/4faab2f1b697827b93e16cb798b22b197e5235c9/testing/mozbase/moztest/moztest/resolve.py#335
I made the above changes locally, and now "mach test" works as expected.
In the described implementation, only the parent manifest is included in the key. This means that the issue may re-appear if there is multiple manifests are nested, with different variables at the top.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 8•6 years ago
|
||
Test manifests may be included by multiple other manifests, optionally
with additional variables below the [include:...]
section header.
These additional variables are specific to the manifest that contained
the "include" section, and should not inadvertently be shared with other
manifests that also happen to include this manifest.
To achieve that, store the defaults for included manifests in a (path to
parent manifest, path to included manifest) tuple instead of just the
included manifest.
Assignee | ||
Comment 9•6 years ago
|
||
I used the following minimal test case for manual reproduction:
Add the following to toolkit/components/extensions/test/xpcshell/xpcshell-common.ini :
[test_ext_dummy.js]
Create toolkit/components/extensions/test/xpcshell/test_ext_dummy.js with:
add_task(() => { ok(true, "file ran"); });
Output (before patch):
0:00.98 TEST_START: toolkit/components/extensions/test/xpcshell/test_ext_dummy.js
0:00.98 TEST_END: SKIP
0:00.98 TEST_START: toolkit/components/extensions/test/xpcshell/test_ext_dummy.js
0:01.08 TEST_END: SKIP
Output (after patch):
0:00.98 TEST_START: toolkit/components/extensions/test/xpcshell/test_ext_dummy.js
0:00.98 TEST_END: SKIP
0:00.98 TEST_START: toolkit/components/extensions/test/xpcshell/test_ext_dummy.js
0:01.08 TEST_END: PASS
The first skipped test is because xpcshell.ini
contains skip-if = os != 'android'
.
The second test is unconditional, from xpcshell-remote.ini
.
The results still differ from mach xpcshell-test
, since mach xpcshell-test
seems to ignore any skip-if
rules (so it runs both tests). I don't know whether this behavior of mach xpcshell-test
is expected, but I'm already glad that at least tests without skip-if
are being run.
Comment 10•6 years ago
|
||
Comment 11•6 years ago
|
||
Backed out for multiple browser-chrome failures e.g. browser_ext_browserAction_popup_resize.js
backout: https://hg.mozilla.org/integration/autoland/rev/70eed5b291e743debe9ade6f101aba97b7131518
failure log example: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=225852683&repo=autoland&lineNumber=1577
[task 2019-02-04T12:36:17.181Z] 12:36:17 INFO - TEST-START | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup_resize.js
[task 2019-02-04T12:36:17.198Z] 12:36:17 INFO - TEST-INFO | started process screentopng
[task 2019-02-04T12:36:17.872Z] 12:36:17 INFO - TEST-INFO | screentopng: exit 0
[task 2019-02-04T12:36:17.874Z] 12:36:17 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup_resize.js | Exception thrown - Error opening input stream (invalid filename?): chrome://mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/head_browserAction.js
[task 2019-02-04T12:36:17.878Z] 12:36:17 INFO - GECKO(1097) | MEMORY STAT | vsize 20974030MB | residentFast 1209MB
[task 2019-02-04T12:36:17.879Z] 12:36:17 INFO - TEST-OK | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup_resize.js | took 57ms
[task 2019-02-04T12:36:17.880Z] 12:36:17 INFO - checking window state
[task 2019-02-04T12:36:17.882Z] 12:36:17 INFO - TEST-START | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup_resize_bottom.js
[task 2019-02-04T12:36:17.883Z] 12:36:17 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-02-04T12:36:17.886Z] 12:36:17 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup_resize_bottom.js | Exception thrown - Error opening input stream (invalid filename?): chrome://mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/head_browserAction.js
[task 2019-02-04T12:36:17.887Z] 12:36:17 INFO - GECKO(1097) | MEMORY STAT | vsize 20974036MB | residentFast 1204MB
[task 2019-02-04T12:36:17.890Z] 12:36:17 INFO - TEST-OK | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_browserAction_popup_resize_bottom.js | took 62ms
Assignee | ||
Updated•6 years ago
|
Comment 12•6 years ago
|
||
Comment 13•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Comment 14•6 years ago
|
||
bugherder uplift |
Comment 15•6 years ago
|
||
Updated•6 years ago
|
Description
•