948801 - Move browser tests from toolkit/mozapps/extensions/test/browser/Makefile.in to browser.ini

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

toolkit/mozapps/extensions/test/browser has browser-chrome tests specified in Makefile.in - they should be specified in a new browser.ini

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug-948801-move-browser-chrome-tests-in-toolkit-moza.patch

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8345696 [details] [diff] [review]
Bug-948801-move-browser-chrome-tests-in-toolkit-moza.patch

Review of attachment 8345696 [details] [diff] [review]:
-----------------------------------------------------------------

You're breaking the MOCHITEST_BROWSER_MAIN installation rules.

Comment 3

[(not currently active) Ted Mielczarek]

He's right. I have no idea what this Makefile is doing, this stuff came from Mossop in bug 597178. Mossop: what the hell is this doing?

Comment 4

[Mark Hammond [:markh] [:mhammond]]

FWIW, mxr tells me that MOCHITEST_BROWSER_MAIN appears only in this Makefile.  A try run with this patch works fine - so I suspect there aren't actually any MOCHITEST_BROWSER_MAIN installation rules being broken.

Comment 5

[Dave Townsend [:mossop]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> He's right. I have no idea what this Makefile is doing, this stuff came from
> Mossop in bug 597178. Mossop: what the hell is this doing?

What should be happening is the tests in MOCHITEST_BROWSER_MAIN_FILES should be getting installed to two different places so they are run twice during the browser chrome run. One set tests the case where the add-ons manager appears in a tab in the browser, the other set tests the case where the add-ons manager runs as a standalone window. Looking at a recent build it looks like that is still happening but the machanism that does it changed in bug 917622 so ping glandium if you have questions about that.

Comment 6

[(not currently active) Ted Mielczarek]

Sounds like we need to use a duplicate manifest like we do for the addons xpcshell tests that run packed and unpacked:
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/xpcshell-unpack.ini

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: move browser-chrome tests in toolkit/mozapps/extensions/test/browser from Makefile.in to browser.ini

Hey, I bet jmaher wants to review more Mochitest manifest patches!

On the plus side, this looks to be the last set of browser-chrome tests that were not in a manifest, so yay!

This relies on the patch from bug 948801 to make the "install-to-subdir" key in the manifest work. This is horrible, and I hope to remove this once we're running tests from manifests.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8382468 [details] [diff] [review]
move browser-chrome tests in toolkit/mozapps/extensions/test/browser from Makefile.in to browser.ini

Review of attachment 8382468 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/test/browser/browser-window.ini
@@ +1,2 @@
> +[DEFAULT]
> +install-to-subdir = test-window

I think it is technically called browser-window, not test-window- this needs a try server run for sure :)

Comment 9

[(not currently active) Ted Mielczarek]

(In reply to Joel Maher (:jmaher) from comment #8)
> I think it is technically called browser-window, not test-window- this needs
> a try server run for sure :)

You are correct, this is going to wind up in a different directory, it'll be $objdir/_tests/testing/mochitest/browser/toolkit/mozapps/extensions/test/browser/test-window vs. the original $objdir/_tests/testing/mochitest/browser/toolkit/mozapps/extensions/test/browser-window .

However, the relevant bit of the tests should be fine since it just looks for "-window" in the last directory in the URL:
http://hg.mozilla.org/mozilla-central/annotate/aa54ebb903ee/toolkit/mozapps/extensions/test/browser/head.js#l16

Comment 10

[(not currently active) Ted Mielczarek]

Unfortunately these are orange on Try, need to figure that out:
https://tbpl.mozilla.org/?tree=Try&rev=ba2a7fc59025

Comment 11

[(not currently active) Ted Mielczarek]

Attachment: move browser-chrome tests in toolkit/mozapps/extensions/test/browser from Makefile.in to browser.ini.

I figured it out. That head.js also tries to normalize the path to support files by popping the last directory off of the URL and replacing it with /browser/, which doesn't work since I moved the tests to a subdirectory. I tweaked it to generate the right path and it looks pretty good on Try so far (still waiting for debug results):
https://tbpl.mozilla.org/?tree=Try&rev=a05ce1dc692c

Comment 12

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eea5db7d83d8

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eea5db7d83d8