All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in mozilla28

Status

RESOLVED FIXED
5 years ago
8 months ago

People

(Reporter: markh, Assigned: markh)

Tracking

Trunk
mozilla28
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 823743 [details] [diff] [review]
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
Attachment #823743 - Flags: feedback?(jhammel)

Comment 1

5 years ago
(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

5 years ago
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-
(Assignee)

Comment 3

5 years ago
It works by accident - see bug 932552.
(Assignee)

Comment 4

5 years ago
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 5

5 years ago
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+
(Assignee)

Comment 6

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

5 years ago
Whiteboard: [qa-]

Updated

4 years ago
Blocks: 773349

Updated

8 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.