Bug 119552 - [CSS Grid Layout] Refactor testing code
Summary: [CSS Grid Layout] Refactor testing code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: BlinkMergeCandidate
Depends on:
Blocks: 60731 119801
  Show dependency treegraph
 
Reported: 2013-08-07 10:46 PDT by Sergio Villar Senin
Modified: 2013-08-22 02:32 PDT (History)
7 users (show)

See Also:


Attachments
Patch (72.67 KB, patch)
2013-08-07 10:58 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (112.09 KB, patch)
2013-08-08 07:46 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (111.56 KB, patch)
2013-08-09 02:43 PDT, Sergio Villar Senin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2013-08-07 10:46:48 PDT
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.
Comment 1 Sergio Villar Senin 2013-08-07 10:47:56 PDT
Actually I found that jchaffraix did exactly this in Blink in r148922.
Comment 2 Sergio Villar Senin 2013-08-07 10:58:04 PDT
Created attachment 208284 [details]
Patch
Comment 3 Sergio Villar Senin 2013-08-07 11:03:14 PDT
(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.
Comment 4 Sergio Villar Senin 2013-08-08 07:46:56 PDT
Created attachment 208343 [details]
Patch
Comment 5 Sergio Villar Senin 2013-08-08 07:48:09 PDT
(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.
Comment 6 Philippe Normand 2013-08-08 08:51:42 PDT
Although this is not really my usual area of expertise I think this refactoring looks quite good :)
Comment 7 Darin Adler 2013-08-08 11:01:11 PDT
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?
Comment 8 Sergio Villar Senin 2013-08-09 00:32:40 PDT
(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).
Comment 9 Sergio Villar Senin 2013-08-09 01:35:03 PDT
(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()...
Comment 10 Sergio Villar Senin 2013-08-09 02:43:03 PDT
Created attachment 208410 [details]
Patch

New version with improved output
Comment 11 Sergio Villar Senin 2013-08-12 07:40:59 PDT
(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.
Comment 12 Sergio Villar Senin 2013-08-16 02:12:38 PDT
(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
Comment 13 Sergio Villar Senin 2013-08-20 03:37:55 PDT
(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...
Comment 14 Darin Adler 2013-08-20 09:56:36 PDT
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.
Comment 15 Sergio Villar Senin 2013-08-22 02:27:21 PDT
Committed r154438: <http://trac.webkit.org/changeset/154438>
Comment 16 Sergio Villar Senin 2013-08-22 02:32:42 PDT
(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)