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

Status: RESOLVED FIXED

Description

[Rob Wu [:robwu]]

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

Comment 1

[Rob Wu [:robwu]]

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

[Tomislav Jovanovic :zombie]

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.

Comment 3

[Geoff Brown [:gbrown]]

(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

[Geoff Brown [:gbrown]]

(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

[Geoff Brown [:gbrown]]

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

[Geoff Brown [:gbrown]]

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

Comment 7

[Rob Wu [:robwu]]

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.

Comment 8

[Rob Wu [:robwu]]

Attachment: Bug 1498636 - Separate "include" variables from manifest defaults

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.

Comment 9

[Rob Wu [:robwu]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4fcae0e31524
Separate "include" variables from manifest defaults r=ahal

Comment 11

[Natalia Csoregi [:nataliaCs]]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/314d22526300
Separate "include" variables from manifest defaults r=ahal

Comment 13

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/314d22526300

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fc9047475850

Comment 15

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/a610a1557958dd97593f15a85969a2b5e1f15d10