"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)
The tests in xpcshell-common.ini and xpcshell-content.ini are always skipped (at least on Linux) when I use "mach test". Using "mach xpcshell-test" works fine somehow. Failing examples (randomly selected): # https://searchfox.org/mozilla-central/rev/28fe656de6/toolkit/components/extensions/test/xpcshell/xpcshell-content.ini#3 mach test toolkit/components/extensions/test/xpcshell/test_ext_alarms.js # https://searchfox.org/mozilla-central/rev/28fe656de6/toolkit/components/extensions/test/xpcshell/xpcshell-common.ini#1 mach test toolkit/components/extensions/test/xpcshell/test_ext_i18n_css.js # Output of the last command: 0:00.39 SUITE_START: xpcshell - running 3 tests 0:00.39 TEST_START: xpcshell-e10s.ini:toolkit/components/extensions/test/xpcshell/test_ext_i18n_css.js 0:00.39 TEST_END: SKIP 0:00.39 TEST_START: xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_i18n_css.js 0:00.39 TEST_END: SKIP 0:00.39 TEST_START: xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_i18n_css.js 0:00.39 TEST_END: SKIP Passing examples: - The above, but "mach test" replaced with "mach xpcshell-test" # https://searchfox.org/mozilla-central/rev/28fe656de6/toolkit/components/extensions/test/xpcshell/xpcshell-remote.ini#15 mach test toolkit/components/extensions/test/xpcshell/test_ext_contentscript_perf_observers.js # https://searchfox.org/mozilla-central/rev/28fe656de6/toolkit/components/extensions/test/xpcshell/xpcshell.ini#33 mach test toolkit/components/extensions/test/xpcshell/test_csp_validator.js
Assignee | ||
Comment 1•2 years ago
|
||
In bug 1496532, toolkit/components/extensions/test/xpcshell/xpcshell.ini was changed: Original: [include:xpcshell-common.ini] skip-if = os == 'win' # Windows extensions always run OOP [include:xpcshell-content.ini] skip-if = os == 'win' # Windows extensions always run OOP The two skip-ifs were replaced with: skip-if = os != 'android' # only android is left without e10s The tests run fine again (on Linux) when I change the skip-ifs to: skip-if = os != 'linux' && os != 'android' # only android is left without e10s
Comment 2•2 years ago
|
||
This is not a regression from bug 1496532, and should not block it. It's an issue with our testing infrastructure that was already present on Windows (similar to bug 1495311), which the change only exposed on other desktop platforms. And since nobody on staff uses Windows, issues like this tend to go unnoticed.
Updated•2 years ago
|
![]() |
||
Updated•2 years ago
|
![]() |
||
Comment 3•2 years ago
|
||
(In reply to Rob Wu [:robwu] from comment #0) > The tests in xpcshell-common.ini and xpcshell-content.ini are always skipped > (at least on Linux) when I use "mach test". Using "mach xpcshell-test" works > fine somehow. I see the same behavior. I find it very curious that "mach test" and "mach xpcshell-test" behave differently. This test directory has a complicated and unusual manifest structure. toolkit/components/extensions/test/xpcshell contains manifests: xpcshell.ini xpcshell-e10s.ini xpcshell-remote.ini xpcshell-content.ini xpcshell-common.ini native_messaging.ini Some manifests include others, sometimes conditionally. Some manifests also have default skip-if's. Given the existing manifest structure, xpcshell-common and xpcshell-content tests should not be run from xpcshell.ini, on Linux: https://dxr.mozilla.org/mozilla-central/rev/b3da3f53f8042d6e2e8f90cd0086e354d96ba2fc/toolkit/components/extensions/test/xpcshell/xpcshell.ini#56-59 [include:xpcshell-common.ini] skip-if = os != 'android' # only android is left without e10s [include:xpcshell-content.ini] skip-if = os != 'android' # only android is left without e10s However, xpcshell-content tests (for instance) should run on Linux, via xpcshell-e10s.ini, at least: https://dxr.mozilla.org/mozilla-central/rev/b3da3f53f8042d6e2e8f90cd0086e354d96ba2fc/toolkit/components/extensions/test/xpcshell/xpcshell-e10s.ini#12 [include:xpcshell-content.ini] In debugging, I think I see xpcshell-content.ini tests excluded from xpcshell-e10s.ini based on the skip-if in xpcshell.ini, in "mach test" only -- unexpected. I am trying to track this down; maybe something in TestResolver?
![]() |
||
Comment 4•2 years ago
|
||
(In reply to Geoff Brown [:gbrown] from comment #3) > In debugging, I think I see xpcshell-content.ini tests excluded from > xpcshell-e10s.ini based on the skip-if in xpcshell.ini, in "mach test" only > -- unexpected. I am trying to track this down; maybe something in > TestResolver? If I look at the test-defaults loaded from the pickle file at https://dxr.mozilla.org/mozilla-central/rev/b3da3f53f8042d6e2e8f90cd0086e354d96ba2fc/testing/mozbase/moztest/moztest/resolve.py#323 I see problematic entries, with skip-if data from xpcshell.ini associated with xpcshell-content.ini, for example. Predictably, I see the same data if I examine self.manifest_defaults before the pickle file is generated: https://dxr.mozilla.org/mozilla-central/rev/b3da3f53f8042d6e2e8f90cd0086e354d96ba2fc/python/mozbuild/mozbuild/backend/test_manifest.py#62 For example, the dict entry for 'toolkit/components/extensions/test/xpcshell/xpcshell-content.ini' includes {'skip-if': "os != 'android'"}, which seems to originate in xpcshell.ini.
![]() |
||
Comment 5•2 years ago
|
||
...and that data is added by add_defaults, called via: File "/home/gbrown/src/build/gen_test_backend.py", line 39, in <module> sys.exit(gen_test_backend()) File "/home/gbrown/src/build/gen_test_backend.py", line 35, in gen_test_backend backend.consume(data) File "/home/gbrown/src/python/mozbuild/mozbuild/backend/base.py", line 128, in consume if (not self.consume_object(obj) and File "/home/gbrown/src/python/mozbuild/mozbuild/backend/test_manifest.py", line 54, in consume_object self.add_defaults(obj.manifest) It's worth noting that add_defaults updates self.manifest_defaults['.../xpcshell-content.ini'] multiple times and the first few calls have the correct data; that's overwritten with the problematic data on the final call.
![]() |
||
Comment 6•2 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•2 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•2 years ago
|
Assignee | ||
Comment 8•2 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•2 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•2 years ago
|
||
Pushed by rob@robwu.nl: https://hg.mozilla.org/integration/autoland/rev/4fcae0e31524 Separate "include" variables from manifest defaults r=ahal
Comment 11•2 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•2 years ago
|
Comment 12•2 years ago
|
||
Pushed by rob@robwu.nl: https://hg.mozilla.org/integration/autoland/rev/314d22526300 Separate "include" variables from manifest defaults r=ahal
Comment 13•2 years ago
|
||
bugherder |
Updated•2 years ago
|
Comment 14•2 years ago
|
||
bugherderuplift |
Comment 15•2 years ago
|
||
Updated•2 years ago
|
Description
•