Closed Bug 1498636 Opened 6 years ago Closed 6 years ago

"mach test toolkit/components/extensions/test/xpcshell/*.js" skips many tests, but "mach xpcshell-test" works

Categories

(Testing :: XPCShell Harness, defect, P2)

defect

Tracking

(firefox-esr60 fixed, firefox65 wontfix, firefox66 fixed, firefox67 fixed)

RESOLVED FIXED
mozilla67
Tracking Status
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
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
Blocks: 1496532
Keywords: regression
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.
Component: General → XPCShell Harness
Product: WebExtensions → Testing
Assignee: nobody → gbrown
Priority: -- → P2
(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?
(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.
...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.

Sorry, haven't been able to find time for this. There is something very interesting happening, possibly with far-reaching consequences.

Assignee: gbrown → nobody

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: nobody → rob
Status: NEW → ASSIGNED

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.

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.

Blocks: 1495311
Pushed by rob@robwu.nl: https://hg.mozilla.org/integration/autoland/rev/4fcae0e31524 Separate "include" variables from manifest defaults r=ahal

Backed out for multiple browser-chrome failures e.g. browser_ext_browserAction_popup_resize.js

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4fcae0e31524da7ca7477e076738f9cf826939bd&searchStr=browser-chrome&group_state=expanded

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

Flags: needinfo?(rob)
Flags: needinfo?(rob)
Pushed by rob@robwu.nl: https://hg.mozilla.org/integration/autoland/rev/314d22526300 Separate "include" variables from manifest defaults r=ahal
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Whiteboard: [checkin-needed-beta][checkin-needed-esr60]
Whiteboard: [checkin-needed-beta][checkin-needed-esr60] → [checkin-needed-esr60]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: