Bug 87991 – Computed style tests are a maintenance burden on the project

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

[Tony Chang]

The computed style tests enumerate the computed CSS properties.  The tests in particular are these:

fast/css/getComputedStyle/computed-style-without-renderer.html (6 results)
fast/css/getComputedStyle/computed-style.html (7 results)
svg/css/getComputedStyle-basic.xhtml (12 results)

The following seem to be problems:
1) The 3 tests are similar and require similar -expected.txt changes.
2) These tests aren't able to share results across platforms since different platforms have different features enabled.  The property-names.js file tries to work around this, but it doesn't seem to be getting much use.
3) svg/css/getComputedStyle-basic.xhtml exposes the CSS OM, which isn't well spec'ed.  It makes the test output more verbose and harder to update.

Some possible solutions:
1) Maybe we can merge computed-style-without-renderer.html and computed-style.html into a single test?  It would reduce the number of -expected files to update, although would require 2 changes to each one.  I think that's a small improvement.
2) We could do a pass over all the platforms and update property-names.js's propertiesToSkip list.  Maybe once we're down to a single expectations, people will start using the property-names.js file.  The down side is that the propertiesToSkip list would likely be large.
3) We could convert svg/css/getComputedStyle-basic.xhtml to just list property names and initial values like the fast/css tests.

[Tony Chang]

Created attachment 147438 [details]
Patch

[Tony Chang]

I decided to use a whitelist like we did in bug 85430 and bug 85526.

[Ryosuke Niwa]

Comment on attachment 147438 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147438&action=review

> LayoutTests/fast/css/getComputedStyle/resources/property-names.js:1
> +// We only test properties that are exposed in all ports.

Doesn't this mean that people need to remember to add values to this test when they add new properties? That seems undesirable. I don't think most people would remember to do that.

[Tony Chang]

(In reply to comment #3)
> (From update of attachment 147438 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147438&action=review
> 
> > LayoutTests/fast/css/getComputedStyle/resources/property-names.js:1
> > +// We only test properties that are exposed in all ports.
> 
> Doesn't this mean that people need to remember to add values to this test when they add new properties? That seems undesirable. I don't think most people would remember to do that.

I think anyone adding a new property will be adding a new layout test that tests the new property.  They don't have to update this test.

[Ryosuke Niwa]

(In reply to comment #4)
> I think anyone adding a new property will be adding a new layout test that tests the new property.  They don't have to update this test.

But then when are we ever going to add a new property to this test? Never? If so, why are we arbitrarily picking a set of properties to test now?  I think this test is similar to binding tests in the sense that it just gives us the state of the world.  If we believe that all these properties are adequately tested elsewhere, maybe we should just remove it altogether.

[Tony Chang]

(In reply to comment #5)
> (In reply to comment #4)
> > I think anyone adding a new property will be adding a new layout test that tests the new property.  They don't have to update this test.
> 
> But then when are we ever going to add a new property to this test? Never?

You can either add a new property to this test or you can write a new test. Both seem acceptable to WebKit testing practices.

> If so, why are we arbitrarily picking a set of properties to test now?  I think this test is similar to binding tests in the sense that it just gives us the state of the world. If we believe that all these properties are adequately tested elsewhere, maybe we should just remove it altogether.

Some of it is redundant with existing tests, some of it isn't. I suspect properties that were recently added don't need to be tested, but I don't know what properties those are. This maintains existing coverage, while not requiring edits to this test going forward.

[Eric Seidel (no email)]

Comment on attachment 147438 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147438&action=review

> LayoutTests/fast/css/getComputedStyle/computed-style-expected.txt:-45
> -font-family: Times;

Why are a few of these going away?

[Tony Chang]

Comment on attachment 147438 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147438&action=review

>> LayoutTests/fast/css/getComputedStyle/computed-style-expected.txt:-45
>> -font-family: Times;
> 
> Why are a few of these going away?

font-family varies on different platforms.  The other values are for features that are disabled on at least one platform.  I'm sure there are other places where we test the properties that are going away.

[Eric Seidel (no email)]

Comment on attachment 147438 [details]
Patch

OK.  We do lose some coverage here.  But the maintinence win is likely worth it.

[Tony Chang]

Created attachment 148645 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 148645 [details]
Patch for landing

Clearing flags on attachment: 148645

Committed r120870: <http://trac.webkit.org/changeset/120870>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.