Bug 32419 – Rewrite CSS namespaces layout tests with dumpAsText

• Status: RESOLVED
• Resolution: INVALID
• Component: Tools / Tests

[Kinuko Yasuda]

Two new layout tests are added recently for CSS namespaces need rebaselining for other platforms, but looks like they can be written with dumpAsText.
fast/css/namespaces/namespaces-comments.xml
fast/css/namespaces/namespaces-empty.xml

[Kinuko Yasuda]

Created attachment 44668 [details]
Patch v1

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44668 [details] without any errors.

[Kent Tamura]

Comment on attachment 44668 [details]
Patch v1

> +        * fast/css/namespaces/script-tests/TEMPLATE.html: Copied from LayoutTests/fast/backgrounds/repeat/script-tests/TEMPLATE.html.
> +        * fast/css/namespaces/script-tests/namespaces-comments.js: Added.
> +        (testCSSNamespace):
> +        * fast/css/namespaces/script-tests/namespaces-empty.js: Added.
> +        (testCSSNamespace):

Did you generate namespaces-*.xhtml by WebKitTools/Scripts/make-script-wrappers ?
If not, you shouldn't put *.js to script-tests directory.

[Kinuko Yasuda]

Created attachment 44678 [details]
Patch v2: moved some files script-tests -> resources

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44678 [details] without any errors.

[Kinuko Yasuda]

(In reply to comment #3)
> (From update of attachment 44668 [details])
> > +        * fast/css/namespaces/script-tests/TEMPLATE.html: Copied from LayoutTests/fast/backgrounds/repeat/script-tests/TEMPLATE.html.
> > +        * fast/css/namespaces/script-tests/namespaces-comments.js: Added.
> > +        (testCSSNamespace):
> > +        * fast/css/namespaces/script-tests/namespaces-empty.js: Added.
> > +        (testCSSNamespace):
> 
> Did you generate namespaces-*.xhtml by WebKitTools/Scripts/make-script-wrappers
> ?
> If not, you shouldn't put *.js to script-tests directory.

Ah, I modified them after generating xhtml files... changed the script
locations from script-tests to resources (is this the expected location?) as I
want to include different stylesheets for each.
Thanks for your super-quick review,

[Kent Tamura]

> Ah, I modified them after generating xhtml files... changed the script
> locations from script-tests to resources (is this the expected location?) as I
> want to include different stylesheets for each.

Yeah, resources is the appropriate place.

[Maciej Stachowiak]

Comment on attachment 44678 [details]
Patch v2: moved some files script-tests -> resources

I don't think the new tests are equivalent to the old ones. Here are some differences:

1) The original tests have the relevant styles inline, not in an external stylesheets included via ?xml-stylesheet PI.
2) The original tests use elements that are present as markup in the document as the test subject.
3) Some of the tests had a non-HTML root element and in general a different tree structure than what your test creates.

All of these changes alter the code paths tested by the tests.

Furthermore, these tests are imported from the official CSS Namespaces test suite. I don't think we should be making such substantial changes to tests from an external test suite. I think it would be ok to add the new text-dumping tests but I do not think the old ones should be removed.

[Kinuko Yasuda]

Let me close this one as invalid.  I can fix the patch to make it dump text but with less substantial changes from the original one (by addressing (1), (2) and (3) Maciej has pointed out), but if we want to keep the original one probably that would be enough.
Thanks for reviewing and sorry for leaving this for a while.