Closed Bug 932147 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla28

People

(Reporter: markh, Assigned: markh)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file)

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
Attachment #823743 - Flags: feedback?(jhammel)
(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 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.
Attachment #823743 - Flags: feedback?(jhammel) → feedback-
It works by accident - see bug 932552.
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.
Attachment #823743 - Flags: review?(jhammel)
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.
Attachment #823743 - Flags: review?(jhammel) → review+
https://hg.mozilla.org/integration/fx-team/rev/dfe65b31de6a
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/dfe65b31de6a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
Blocks: 773349
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.