932147 - Move remaining tests in browser/base/content/test/general/Makefile.in to manifest files.

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Attachment: Move-remaining-tests-in-browser-base-content-test-ge.patch

browser/base/content/test/general has most tests specified in manifest, but some test which are disabled in certain configurations remain in Makefile.in.  These remaining tests should all be moved to the relevant manifest with skip-if or run-if conditions to express these conditions.

This is blocking the e10s test work (bug 932142) as a couple of tests which are still in the Makefile also need to be disabled when e10s is active - this bug will allow us to add a new condition so these tests are skipped when they currently are *and* on all platforms when e10s is enabled.  It depends on bug 930849 as it uses the new 'datareporting' condition introduced there.

Requesting feedback (I picked jhammel almost at random - please pass on to someone else if necessary) as I'm not sure about some of the conditions - eg, where the Makefile.in had:

# test_contextmenu.html and test_contextmenu_input are disabled on Linux due to bug 513558
ifndef MOZ_WIDGET_GTK
...

I made the condition:

skip-if = toolkit == "gtk2" || toolkit == "gtk3" # disabled on Linux due to bug 513558

which (hopefully) matches the condition in the Makefile, but the comment implies it could just test the |os| variable?

Try at https://tbpl.mozilla.org/?tree=Try&rev=1dc441a0d938

Comment 1

[Ed Morley [:emorley]]

(In reply to Mark Hammond [:markh] from comment #0)
> I made the condition:
> 
> skip-if = toolkit == "gtk2" || toolkit == "gtk3" # disabled on Linux due to
> bug 513558
> 
> which (hopefully) matches the condition in the Makefile, but the comment
> implies it could just test the |os| variable?

These were disabled by http://hg.mozilla.org/mozilla-central/rev/24b9290db3a4 - bug 622284 comment 2 has the only further info I could find on it.

Comment 2

[Jeff Hammel]

Comment on attachment 823743 [details] [diff] [review]
Move-remaining-tests-in-browser-base-content-test-ge.patch

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

f- due to comment concerns (unless I am wrong).  Otherwise, looks good.

::: browser/base/content/test/general/browser.ini
@@ +88,5 @@
>  
> +[browser_CTP_context_menu.js]
> +skip-if = toolkit == "gtk2" || toolkit == "gtk3"   # browser_CTP_context_menu.js fails intermittently on Linux (bug 909342)
> +[browser_CTP_crashreporting.js]
> +run-if = crashreporter

Do comments on the same line work? Casual reading says no:

https://github.com/mozilla/mozbase/blob/master/manifestdestiny/manifestparser/manifestparser.py#L185
https://github.com/mozilla/mozbase/blob/master/manifestdestiny/manifestparser/manifestparser.py#L331

I'll admit this is horribly underdocumented.

Comment 3

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

It works by accident - see bug 932552.

Comment 4

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 823743 [details] [diff] [review]
Move-remaining-tests-in-browser-base-content-test-ge.patch

Requesting review now that we have the comment issue addressed.

Comment 5

[Jeff Hammel]

Comment on attachment 823743 [details] [diff] [review]
Move-remaining-tests-in-browser-base-content-test-ge.patch

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

> f- due to comment concerns (unless I am wrong).  Otherwise, looks good.

Addressed :) Looks good!

::: browser/base/content/test/general/browser.ini
@@ +88,5 @@
>  
> +[browser_CTP_context_menu.js]
> +skip-if = toolkit == "gtk2" || toolkit == "gtk3"   # browser_CTP_context_menu.js fails intermittently on Linux (bug 909342)
> +[browser_CTP_crashreporting.js]
> +run-if = crashreporter

Do comments on the same line work? Casual reading says no:

https://github.com/mozilla/mozbase/blob/master/manifestdestiny/manifestparser/manifestparser.py#L185
https://github.com/mozilla/mozbase/blob/master/manifestdestiny/manifestparser/manifestparser.py#L331

I'll admit this is horribly underdocumented.

Comment 6

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

https://hg.mozilla.org/integration/fx-team/rev/dfe65b31de6a

Comment 7

[Carsten Book [:Tomcat]]

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