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

RESOLVED FIXED in Firefox -esr60

Status

defect
P2
normal
RESOLVED FIXED
8 months ago
4 months ago

People

(Reporter: robwu, Assigned: robwu)

Tracking

({regression})

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

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

Details

Attachments

(1 attachment)

Assignee

Description

8 months ago
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

8 months 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
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.
No longer blocks: 1496532
See Also: → 1495311
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
Assignee

Comment 7

5 months 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

5 months ago
Assignee: nobody → rob
Status: NEW → ASSIGNED
Assignee

Comment 8

5 months 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

5 months 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.

Assignee

Updated

5 months ago
Blocks: 1495311

Comment 10

4 months ago
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)
Assignee

Updated

4 months ago
Flags: needinfo?(rob)

Comment 12

4 months ago
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/314d22526300
Separate "include" variables from manifest defaults r=ahal

Comment 13

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Whiteboard: [checkin-needed-beta][checkin-needed-esr60]

Comment 14

4 months ago
bugherderuplift
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.