Bug 119552

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

Created attachment 208284 [details] Patch

(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.

Created attachment 208343 [details] Patch

(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.

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

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?

(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).

(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()...

Created attachment 208410 [details] Patch New version with improved output

(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.

(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

(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 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.

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

(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)