Move some mochitests in layout/ to manifests

RESOLVED FIXED in mozilla28

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

unspecified
mozilla28
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 833448 [details] [diff] [review]
Part a: layout/generic and layout/style
Attachment #833448 - Flags: review?(cam)
(Assignee)

Comment 2

5 years ago
Created attachment 833449 [details] [diff] [review]
Part b: layout/base
Attachment #833449 - Flags: review?(cam)
(Assignee)

Comment 3

5 years ago
Created attachment 833456 [details] [diff] [review]
Part b: layout/base
Attachment #833449 - Attachment is obsolete: true
Attachment #833449 - Flags: review?(cam)
Attachment #833456 - Flags: review?(cam)
Is there documentation on the new manifest format?  I haven't worked with it yet.
Comment on attachment 833448 [details] [diff] [review]
Part a: layout/generic and layout/style

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

::: layout/generic/test/Makefile.in
@@ -3,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> -# in the list below, we make sure that the tests that require focus
> -# run before test_plugin_clipping, which can steal focus for its window.

Is this comment not accurate any more?  If it's still accurate, should you copy it across?

::: layout/generic/test/mochitest.ini
@@ +1,3 @@
> +[DEFAULT]
> +support-files =
> +  bug344830_testembed.svg

Why not list this one under [test_bug344830.html]?

@@ +33,5 @@
> +[test_bug405178.html]
> +[test_bug416168.html]
> +[test_bug421436.html]
> +[test_bug421839-1.html]
> +skip-if = true # Disabled for calling finish twice

http://mozbase.readthedocs.org/en/latest/manifestdestiny.html says comments must take up a whole line, and can't be at the end of a line like this.  Are the docs wrong, or does this not work here?

@@ +39,5 @@
> +support-files = bug421839-2-page.html
> +[test_bug424627.html]
> +[test_bug438840.html]
> +[test_bug448860.html]
> +support-files = file_bug448987.html file_bug448987_ref.html file_bug448987_notref.html

Should this support-files be two lines down?

@@ +88,5 @@
> +# Disable the caret movement by word test on Linux because the shortcut keys
> +# are defined in system level.  So, it depends on the environment.
> +skip-if = (widget == "gtk2") || (widget == "gtk3")
> +[test_page_scroll_with_fixed_pos.html]
> +support-files = page_scroll_with_fixed_pos_window.html]

Closing square bracket seems wrong.  What's the consequence of getting support-files wrong?

@@ +99,5 @@
> +[test_plugin_position.xhtml]
> +[test_selection_expanding.html]
> +[test_selection_splitText-normalize.html]
> +[test_selection_touchevents.html]
> +support-files = selection_expanding_xbl.xml

Should this support-files line be two lines up?
Attachment #833448 - Flags: review?(cam) → review-
(Assignee)

Comment 7

5 years ago
(In reply to Cameron McCormack (:heycam) from comment #6)
> Comment on attachment 833448 [details] [diff] [review]
> Part a: layout/generic and layout/style
> 
> Review of attachment 833448 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/test/Makefile.in
> @@ -3,5 @@
> >  # License, v. 2.0. If a copy of the MPL was not distributed with this
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> > -# in the list below, we make sure that the tests that require focus
> > -# run before test_plugin_clipping, which can steal focus for its window.
> 
> Is this comment not accurate any more?  If it's still accurate, should you
> copy it across?

The order in the list it refers to doesn't actually affect the order in which the tests run, so I'm pretty sure the comment was misguided.

> ::: layout/generic/test/mochitest.ini
> @@ +1,3 @@
> > +[DEFAULT]
> > +support-files =
> > +  bug344830_testembed.svg
> 
> Why not list this one under [test_bug344830.html]?

Can do

> @@ +33,5 @@
> > +[test_bug405178.html]
> > +[test_bug416168.html]
> > +[test_bug421436.html]
> > +[test_bug421839-1.html]
> > +skip-if = true # Disabled for calling finish twice
> 
> http://mozbase.readthedocs.org/en/latest/manifestdestiny.html says comments
> must take up a whole line, and can't be at the end of a line like this.  Are
> the docs wrong, or does this not work here?

It seems to work, but I'll double-check.

> @@ +39,5 @@
> > +support-files = bug421839-2-page.html
> > +[test_bug424627.html]
> > +[test_bug438840.html]
> > +[test_bug448860.html]
> > +support-files = file_bug448987.html file_bug448987_ref.html file_bug448987_notref.html
> 
> Should this support-files be two lines down?
> 
> @@ +88,5 @@
> > +# Disable the caret movement by word test on Linux because the shortcut keys
> > +# are defined in system level.  So, it depends on the environment.
> > +skip-if = (widget == "gtk2") || (widget == "gtk3")
> > +[test_page_scroll_with_fixed_pos.html]
> > +support-files = page_scroll_with_fixed_pos_window.html]
> 
> Closing square bracket seems wrong.  What's the consequence of getting
> support-files wrong?

The build breaks. I fixed this locally, but apparently in the wrong patch.

> @@ +99,5 @@
> > +[test_plugin_position.xhtml]
> > +[test_selection_expanding.html]
> > +[test_selection_splitText-normalize.html]
> > +[test_selection_touchevents.html]
> > +support-files = selection_expanding_xbl.xml
> 
> Should this support-files line be two lines up?

Yes, indeed.
Comment on attachment 833456 [details] [diff] [review]
Part b: layout/base

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

::: layout/base/tests/chrome/chrome.ini
@@ +44,5 @@
>  [test_transformed_scrolling_repaints.html]
>  [test_transformed_scrolling_repaints_2.html]
>  [test_transformed_scrolling_repaints_3.html]
> +[test_leaf_layers_partition_browser_window.xul]
> +skip-if (!debug) || (widget == "cocoa") # Disabled on Mac because of Bug 748219

"skip-if =".  Also keep the list of tests sorted.

::: layout/base/tests/mochitest.ini
@@ +47,5 @@
> +  bug570378-persian-3-ref.html
> +  bug570378-persian-4.html
> +  bug570378-persian-4-ref.html
> +  bug570378-persian-5.html
> +  bug570378-persian-5-ref.html

Can you put these support files that reference bug numbers in their relevant section below?

@@ +61,5 @@
> +[test_bug93077-5.html]
> +[test_bug93077-6.html]
> +[test_bug114649.html]
> +[test_bug369950.html]
> +ski-if = true # Bug 492575

skip-if

@@ +181,5 @@
> +[test_bug842853-2.html]
> +[test_bug849219.html]
> +[test_bug851485.html]
> +[test_bug851445.html]
> +support-files = bug851445_helper.html]

Stray "]".

@@ +191,5 @@
> +# Tests for bugs 441782, 467672 and 570378 do not pass reliably on Windows,
> +# because of bug 469208.
> +ifeq (,$(filter windows,$(MOZ_WIDGET_TOOLKIT)))
> +# THESE TESTS (BELOW) DO NOT RUN ON WINDOWS
> +MOCHITEST_FILES +=]

?  Are you using some sort of converter from the existing Makefile?  Also why are these errors not causing the whole mochitest run to fail?

@@ +442,5 @@
> +[test_bug749186.html]
> +skip-if = widget == "win"
> +[test_bug644768.html]
> +skip-if = widget == "win"
> +

rm blank line
Attachment #833456 - Flags: review?(cam) → review-
(Assignee)

Comment 9

5 years ago
(In reply to Cameron McCormack (:heycam) from comment #8)
> Comment on attachment 833456 [details] [diff] [review]
> Part b: layout/base
> 
> Review of attachment 833456 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/tests/mochitest.ini
> @@ +47,5 @@
> > +  bug570378-persian-3-ref.html
> > +  bug570378-persian-4.html
> > +  bug570378-persian-4-ref.html
> > +  bug570378-persian-5.html
> > +  bug570378-persian-5-ref.html
> 
> Can you put these support files that reference bug numbers in their relevant
> section below?

These are support files for multiple tests; there's no good way to encode that.
(Assignee)

Updated

5 years ago
Flags: needinfo?(Ms2ger)
(Assignee)

Comment 10

5 years ago
Created attachment 8339462 [details] [diff] [review]
Part a: move some mochitests in layout/{generic, style} to manifests;
Attachment #8339462 - Flags: review?(cam)
(Assignee)

Comment 11

5 years ago
Created attachment 8339463 [details] [diff] [review]
Part b: move some mochitests in layout/base to manifests;
Attachment #8339463 - Flags: review?(cam)
(Assignee)

Updated

5 years ago
Attachment #833456 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #833448 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Flags: needinfo?(Ms2ger)
Attachment #8339462 - Flags: review?(cam) → review+
Attachment #8339463 - Flags: review?(cam) → review+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/mozilla-central/rev/34864feb4436
https://hg.mozilla.org/mozilla-central/rev/733f30f7093b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.