Bug 119552 – [CSS Grid Layout] Refactor testing code

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sergio Villar Senin]

The current testing code in LayoutTests/fast/css-grid-layout/grid-item-column-row-get-set.html is a nightmare because it's basically a huge copy&paste of large chunks of testing code. Adding a new test case, involves ~10 new JS lines plus ~6 results lines.

We should refactor that in some utility functions.

[Sergio Villar Senin]

Actually I found that jchaffraix did exactly this in Blink in r148922.

[Sergio Villar Senin]

Created attachment 208284 [details]
Patch

[Sergio Villar Senin]

(In reply to comment #2)
> Created an attachment (id=208284) [details]
> Patch

The -expected.txt results are a bit less verbose as we do not show the exact value for grid-{row|column}-{start|end} as we used to (on success I mean). The reason is because the well known shouldBe() function does something like:

  testPassed(_a + 'is' + _b)

so it prints the arguments and not their evaluation. I have another version of the patch where I defined a new shouldBe() function which was printing eval(_b) instead of _b, but I think it does not pay off, because we don't likely want to have those two versions of shouldBe around.

[Sergio Villar Senin]

Created attachment 208343 [details]
Patch

[Sergio Villar Senin]

(In reply to comment #4)
> Created an attachment (id=208343) [details]
> Patch

Added a second round of refactorings that simplify the maintenance of these tests indeed.

[Philippe Normand]

Although this is not really my usual area of expertise I think this refactoring looks quite good :)

[Darin Adler]

Comment on attachment 208343 [details]
Patch

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

> LayoutTests/fast/css-grid-layout/grid-item-column-row-get-set-expected.txt:7
> +PASS getComputedStyle(gridItem, '').getPropertyValue('-webkit-grid-column') is gridColumnValue

This kind of ruins the test output. It is way better to see actual values, unless there is some reason we cannot. Can we preserve the more concrete output, while still improving these tests?

[Sergio Villar Senin]

(In reply to comment #7)
> (From update of attachment 208343 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208343&action=review
> 
> > LayoutTests/fast/css-grid-layout/grid-item-column-row-get-set-expected.txt:7
> > +PASS getComputedStyle(gridItem, '').getPropertyValue('-webkit-grid-column') is gridColumnValue
> 
> This kind of ruins the test output. It is way better to see actual values, unless there is some reason we cannot. Can we preserve the more concrete output, while still improving these tests?

Yeah that's what I mentioned in comment #3. As I mentioned we can keep the old verbose if we use a modified version of shouldBe changing the testPassed() call. Either that or add a new argument to shouldBe to instruct it to print the evaluated version of the second argument (but that seemed a bit ad-hoc to me).

[Sergio Villar Senin]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 208343 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=208343&action=review
> > 
> > > LayoutTests/fast/css-grid-layout/grid-item-column-row-get-set-expected.txt:7
> > > +PASS getComputedStyle(gridItem, '').getPropertyValue('-webkit-grid-column') is gridColumnValue
> > 
> > This kind of ruins the test output. It is way better to see actual values, unless there is some reason we cannot. Can we preserve the more concrete output, while still improving these tests?
> 
> Yeah that's what I mentioned in comment #3. As I mentioned we can keep the old verbose if we use a modified version of shouldBe changing the testPassed() call. Either that or add a new argument to shouldBe to instruct it to print the evaluated version of the second argument (but that seemed a bit ad-hoc to me).

Hmm actually there might be another solution which would be using ShouldBeEqualToString()...

[Sergio Villar Senin]

Created attachment 208410 [details]
Patch

New version with improved output

[Sergio Villar Senin]

(In reply to comment #10)
> Created an attachment (id=208410) [details]
> Patch
> 
> New version with improved output

Darin, are you OK with the latest proposal. It'd be nice to get this reviewed soon as it's kind of blocking further improvements in the grid layout code.

[Sergio Villar Senin]

(In reply to comment #11)
> (In reply to comment #10)
> > Created an attachment (id=208410) [details] [details]
> > Patch
> > 
> > New version with improved output
> 
> Darin, are you OK with the latest proposal. It'd be nice to get this reviewed soon as it's kind of blocking further improvements in the grid layout code.

Darin ping

[Sergio Villar Senin]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Created an attachment (id=208410) [details] [details] [details]
> > > Patch
> > > 
> > > New version with improved output
> > 
> > Darin, are you OK with the latest proposal. It'd be nice to get this reviewed soon as it's kind of blocking further improvements in the grid layout code.
> 
> Darin ping

Another ping...

[Darin Adler]

Comment on attachment 208410 [details]
Patch

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

> LayoutTests/fast/css-grid-layout/grid-item-column-row-get-set-expected.txt:15
> +PASS getComputedStyle(gridItem, '').getPropertyValue('-webkit-grid-column') is "10 / auto"

Can’t we preserve the “gridItemWithPositiveInteger” that we had before in here? Seems like we could just pass a string in and get it just like we did before.

[Sergio Villar Senin]

Committed r154438: <http://trac.webkit.org/changeset/154438>

[Sergio Villar Senin]

(In reply to comment #14)
> (From update of attachment 208410 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208410&action=review
> 
> > LayoutTests/fast/css-grid-layout/grid-item-column-row-get-set-expected.txt:15
> > +PASS getComputedStyle(gridItem, '').getPropertyValue('-webkit-grid-column') is "10 / auto"
> 
> Can’t we preserve the “gridItemWithPositiveInteger” that we had before in here? Seems like we could just pass a string in and get it just like we did before.

The previous patch added a few extra debug lines to allow us to know which item was being processed in order to improve that. Anyway I managed to get the same output as the original one so we'll preserve the same level of verbosity (actually the test results shouldn't have changed except for the fact that we now have double instead of single quotes)