1286016 - support-files not included for tests in toolkit/mozapps/extensions/test/browser/browser.ini

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

+++ This bug was initially created as a clone of Bug #1285618 +++

$ ./mach test toolkit/mozapps/extensions/test/browser/browser_webapi_addon_listener.js

This fails because support-files aren't being installed.

toolkit/mozapps/extensions/test/browser/browser.ini includes browser-common.ini which has a [DEFAULT] section containing support-files.

Debugging code in testing.py, the Python objects describing the tests (as resolved by testing.py:resolve_tests()) don't contain support-files entries.

Here is the entry from all-tests.json:

 "toolkit/mozapps/extensions/test/browser/browser_webapi_addon_listener.js": [
            {
                "dir_relpath": "toolkit/mozapps/extensions/test/browser",
                "file_relpath": "toolkit/mozapps/extensions/test/browser/browser_webapi_addon_listener.js",
                "flavor": "browser-chrome",
                "here": "/home/gps/src/firefox/toolkit/mozapps/extensions/test/browser",
                "manifest": "/home/gps/src/firefox/toolkit/mozapps/extensions/test/browser/browser.ini",
                "name": "browser_webapi_addon_listener.js",
                "path": "/home/gps/src/firefox/toolkit/mozapps/extensions/test/browser/browser_webapi_addon_listener.js",
                "relpath": "toolkit/mozapps/extensions/test/browser/browser_webapi_addon_listener.js",
                "skip-if": "buildapp == 'mulet'",
                "subsuite": "",
                "tags": "addons"
            }
        ],

Tests defined in browser-common.ini do have support-files set. e.g.

 "toolkit/mozapps/extensions/test/browser/browser_bug562797.js": [
            {
                "ancestor-manifest": "/home/gps/src/firefox/toolkit/mozapps/extensions/test/browser/browser-window.ini",
                "dir_relpath": "toolkit/mozapps/extensions/test/browser",
                "file_relpath": "toolkit/mozapps/extensions/test/browser/browser_bug562797.js",
                "flavor": "browser-chrome",
                "here": "/home/gps/src/firefox/toolkit/mozapps/extensions/test/browser",
                "install-to-subdir": "test-window",
                "manifest": "/home/gps/src/firefox/toolkit/mozapps/extensions/test/browser/browser-common.ini",
                "name": "browser_bug562797.js",
                "path": "/home/gps/src/firefox/toolkit/mozapps/extensions/test/browser/browser_bug562797.js",
                "relpath": "toolkit/mozapps/extensions/test/browser/browser_bug562797.js",
                "subsuite": "",
                "support-files": "\naddons/*\naddon_about.xul\naddon_prefs.xul\ncancelCompatCheck.sjs\ndiscovery.html\ndiscovery_frame.html\ndiscovery_install.html\nhead.js\nsigned_hotfix.rdf\nsigned_hotfix.xpi\nunsigned_hotfix.rdf\nunsigned_hotfix.xpi\nmore_options.xul\noptions.xul\nplugin_test.html\nredirect.sjs\nreleaseNotes.xhtml\nblockNoPlugins.xml\nblockPluginHard.xml\nbrowser_bug557956.rdf\nbrowser_bug557956_8_2.xpi\nbrowser_bug557956_9_2.xpi\nbrowser_bug557956.xml\nbrowser_bug591465.xml\nbrowser_bug593535.xml\nbrowser_searching.xml\nbrowser_searching_empty.xml\nbrowser_updatessl.rdf\nbrowser_updatessl.rdf^headers^\nbrowser_install.rdf\nbrowser_install.rdf^headers^\nbrowser_install.xml\nbrowser_install1_3.xpi\nbrowser_eula.xml\nbrowser_purchase.xml\nwebapi_addon_listener.html\nwebapi_checkavailable.html\nwebapi_checkchromeframe.xul\nwebapi_checkframed.html\nwebapi_checknavigatedwindow.html\n!/toolkit/mozapps/extensions/test/xpinstall/corrupt.xpi\n!/toolkit/mozapps/extensions/test/xpinstall/incompatible.xpi\n!/toolkit/mozapps/extensions/test/xpinstall/installtrigger.html\n!/toolkit/mozapps/extensions/test/xpinstall/restartless.xpi\n!/toolkit/mozapps/extensions/test/xpinstall/theme.xpi\n!/toolkit/mozapps/extensions/test/xpinstall/unsigned.xpi\n!/toolkit/mozapps/extensions/test/xpinstall/amosigned.xpi"
            },

I'm not sure if this bug is in all-tests.json not being written correctly or testing.py not picking up support-files from inherited manifests properly at test execution time. Either way, this looks like a bug.

Comment 1

[Gregory Szorc [:gps]]

Chris: what are your thoughts on this?

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Comment 0 is consistent with my understanding of how the manifest parser works: browser.ini is including browser-common.ini, so defaults from browser.ini will apply to browser-common.ini, but defaults in browser-common.ini will not apply to browser.ini.

Comment 3

[Andrew Swan [:aswan]]

(In reply to Chris Manchester (:chmanchester) from comment #2)
> Comment 0 is consistent with my understanding of how the manifest parser
> works: browser.ini is including browser-common.ini, so defaults from
> browser.ini will apply to browser-common.ini, but defaults in
> browser-common.ini will not apply to browser.ini.

So it sounds like the only way to make this work correctly is for any shared support files to be duplicated in browser.ini, browser-common.ini and browser-window.ini?

Comment 4

[Andrew Swan [:aswan]]

Not being able to run tests from browser.ini is getting increasingly annoying, can I get a confirmation that duplicating the list of support files is necessary before submitting a patch to do that?

Comment 5

[Chris Manchester (limited bugmail, email directly)]

(In reply to Andrew Swan [:aswan] from comment #4)
> Not being able to run tests from browser.ini is getting increasingly
> annoying, can I get a confirmation that duplicating the list of support
> files is necessary before submitting a patch to do that?

I'm not sure which list this refers to. Given the current implementation of the manifest parser, support files need to be alongside a particular test, or in the default section of that test's manifest or a manifest including that test's manifest.

Which are the tests that can't be run?

Comment 6

[Andrew Swan [:aswan]]

(In reply to Chris Manchester (:chmanchester) from comment #5)
> I'm not sure which list this refers to. Given the current implementation of
> the manifest parser, support files need to be alongside a particular test,
> or in the default section of that test's manifest or a manifest including
> that test's manifest.
> 
> Which are the tests that can't be run?

All the tests defined in toolkit/mozapps/extensions/test/browser/browser.ini (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/browser.ini)
There is a common set of support files, defined in browser-common.ini, which is included by browser.ini.
browser-common.ini also contains individual tests that use the shared support files.
I guess what I'm looking for is a way to declare this list once and have it apply globally to a list of tests that is assembled from multiple .ini files with include directives?

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Ok. One way to achieve that might be to have a single manifest containing the support files that includes both browser.ini and browser-window.ini. Short of that we need to move the list of support files to browser.ini, and refer to them in browser-window.ini as well. Either way the list should be removed from browser-common.ini.

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Re-summarizing to match the specific issue described in comment 6. I'll post a patch this afternoon.

Comment 10

[Chris Manchester (limited bugmail, email directly)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48abcadcd0d3

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1286016 - Re-organize browser.ini manifests to reflect support files needed by tests in toolkit/mozapps/extensions/test/browser/browser.ini

Review commit: https://reviewboard.mozilla.org/r/65666/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65666/

Comment 12

[Chris Manchester (limited bugmail, email directly)]

Eric, feel free to re-direct. Mostly looking for someone to verify the fix at this point.

Comment 13

[Eric Rahm [:erahm]]

Comment on attachment 8773025 [details]
Bug 1286016 - Re-organize browser.ini manifests to reflect support files needed by tests in toolkit/mozapps/extensions/test/browser/browser.ini

https://reviewboard.mozilla.org/r/65666/#review62698

So I can confirm this did fix things for me locally, but I think we need to actually fix the underlying issue (presumably that support-files listed in an included manifest aren't being processed properly).

::: toolkit/mozapps/extensions/test/browser/browser-common.ini
(Diff revision 1)
> -[DEFAULT]
> -support-files =

Moving the shared support-files list out seems like a bad thing (tm).

Comment 14

[Eric Rahm [:erahm]]

FWIW including manifests seems to be a thing that we do: https://dxr.mozilla.org/mozilla-central/search?q=ext%3Aini+%27include%3A%27&redirect=false

Comment 15

[Chris Manchester (limited bugmail, email directly)]

Yes, including manifests is a thing that we do. The problem with these particular manifests is that they assume that an including manifest receives default values from the manifest it includes. As far as I know, this has never been the case, and these manifests need to be fixed to reflect that.

Comment 16

[Eric Rahm [:erahm]]

Comment on attachment 8773025 [details]
Bug 1286016 - Re-organize browser.ini manifests to reflect support files needed by tests in toolkit/mozapps/extensions/test/browser/browser.ini

https://reviewboard.mozilla.org/r/65666/#review62720

We talked about this further, these tests are just a bit odd in how they're setup. :chmanchester noted that `[DEFAULT]` blocks aren't included and that's the root of the issue. I think this fix is good enough and as a followup someone more familiar with the code might want to audit the `support-files` list for each manifest to make sure they're actually needed.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d4f43219dcf
Re-organize browser.ini manifests to reflect support files needed by tests in toolkit/mozapps/extensions/test/browser/browser.ini r=erahm

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8d4f43219dcf