Bug 32419 - Rewrite CSS namespaces layout tests with dumpAsText
Summary: Rewrite CSS namespaces layout tests with dumpAsText
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-11 04:15 PST by Kinuko Yasuda
Modified: 2010-03-04 18:51 PST (History)
2 users (show)

See Also:


Attachments
Patch v1 (36.63 KB, patch)
2009-12-11 04:21 PST, Kinuko Yasuda
no flags Details | Formatted Diff | Diff
Patch v2: moved some files script-tests -> resources (35.83 KB, patch)
2009-12-11 05:18 PST, Kinuko Yasuda
mjs: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kinuko Yasuda 2009-12-11 04:15:14 PST
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
Comment 1 Kinuko Yasuda 2009-12-11 04:21:50 PST
Created attachment 44668 [details]
Patch v1
Comment 2 WebKit Review Bot 2009-12-11 04:24:34 PST
style-queue ran check-webkit-style on attachment 44668 [details] without any errors.
Comment 3 Kent Tamura 2009-12-11 04:34:58 PST
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.
Comment 4 Kinuko Yasuda 2009-12-11 05:18:00 PST
Created attachment 44678 [details]
Patch v2: moved some files script-tests -> resources
Comment 5 WebKit Review Bot 2009-12-11 05:21:36 PST
style-queue ran check-webkit-style on attachment 44678 [details] without any errors.
Comment 6 Kinuko Yasuda 2009-12-11 05:23:53 PST
(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,
Comment 7 Kent Tamura 2009-12-11 05:31:33 PST
> 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.
Comment 8 Maciej Stachowiak 2009-12-29 07:57:51 PST
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.
Comment 9 Kinuko Yasuda 2010-03-04 18:51:18 PST
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.