Bug 204407 – [css-grid] Move some alignment tests to the WPT folder

• Component: Tools / Tests
• Status: RESOLVED FIXED

The following tests have been moved:

grid-align-content.html
grid-align-justify-margin-border-padding-vertical-lr.html
grid-align-justify-margin-border-padding-vertical-rl.html
grid-align-justify-margin-border-padding.html
grid-align-justify-overflow.html
grid-align-justify-stretch-with-orthogonal-flows.html
grid-align-justify-stretch.html
grid-align.html

Created attachment 383957 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 383957 [details]
Patch

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

Thanks for working on this, but you need to do some changes on this patch.

> LayoutTests/ChangeLog:8
> +        Replacing some tests from fast/css-grid/alignment to the web platform test folder

The tests are already upstream in WPT, so this is basically importing them and removing the duplicates in WebKit local tests.
It'd be nice if you explain that on the ChangeLog properly.

> LayoutTests/ChangeLog:10
> +        * fast/css-grid-layout/grid-align-justify-stretch-with-orthogonal-flows.html: Removed.

This doesn't seem accurate.

ChangeLog is auto-generated, so it should include automatically all the tests you're removing.

> LayoutTests/ChangeLog:662
> +        <rdar://problem/56801615>

Your patch shouldn't include changes in other parts of the ChangeLog.
I guess your editor was removing trailing white-spaces or something like that and causing the changes here.

[Manuel Rego Casasnovas]

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

Thanks for working on this, but you need to do some changes on this patch.

> LayoutTests/ChangeLog:8
> +        Replacing some tests from fast/css-grid/alignment to the web platform test folder

The tests are already upstream in WPT, so this is basically importing them and removing the duplicates in WebKit local tests.
It'd be nice if you explain that on the ChangeLog properly.

> LayoutTests/ChangeLog:10
> +        * fast/css-grid-layout/grid-align-justify-stretch-with-orthogonal-flows.html: Removed.

This doesn't seem accurate.

ChangeLog is auto-generated, so it should include automatically all the tests you're removing.

> LayoutTests/ChangeLog:662
> +        <rdar://problem/56801615>

Your patch shouldn't include changes in other parts of the ChangeLog.
I guess your editor was removing trailing white-spaces or something like that and causing the changes here.

[Manuel Rego Casasnovas]

Sorry about the duplicated comment, it was not my intention. O:-)

Created attachment 384049 [details]
Patch

Created attachment 384050 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 384050 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +        * fast/css-grid-layout/grid-align-content.html: Removed.

You need to remove the -expected.txt files too.

> LayoutTests/imported/w3c/ChangeLog:11
> +        * web-platform-tests/css/css-grid/alignment/grid-align-content.html:

We need to include in the patch the new results for the tests.

Created attachment 386098 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 386098 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        Remove some tests from fast/css-grid/alignment, that are being replaced by adapted tests in the corresponding Wpt test folder

Nit: s/Wpt/WPT/
Nit: Add a dot at the end of the sentence.

> LayoutTests/imported/w3c/ChangeLog:8
> +        Add some css alignment tests from WebKit, checked and adapted to Wpt, in the corresponding css-grid/alignment folder.

Nit: s/Wpt/WPT/.

> LayoutTests/imported/w3c/ChangeLog:11
> +        * web-platform-tests/css/css-grid/alignment/grid-align-content.html:

You miss the new -expected.txt results for all these tests.

[Javier Fernandez]

Comment on attachment 386098 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/alignment/grid-align-justify-margin-border-padding-vertical-lr.html:11
> +<link rel="stylesheet" href="/css/support/width-keyword-classes.css">

This test is failing for several cases, probably because this 'css' resource file is not located where you expect.

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/alignment/grid-align-justify-margin-border-padding-vertical-rl.html:11
> +<link rel="stylesheet" href="/css/support/width-keyword-classes.css">

Ditto

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/alignment/grid-align-justify-margin-border-padding.html:10
> +<link rel="stylesheet" href="/css/support/width-keyword-classes.css">

Ditto

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/alignment/grid-align-justify-overflow.html:11
> +<link rel="stylesheet" href="/css/support/width-keyword-classes.css">

Ditto

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/alignment/grid-align-justify-stretch-with-orthogonal-flows.html:11
> +<link rel="stylesheet" href="/css/support/width-keyword-classes.css">

Ditto

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/alignment/grid-align-justify-stretch.html:14
> +<link rel="stylesheet" href="/css/support/width-keyword-classes.css">

Ditto

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/alignment/grid-align.html:12
> +<link rel="stylesheet" href="/css/support/width-keyword-classes.css">

Ditto

> LayoutTests/imported/w3c/web-platform-tests/css/css-grid/alignment/w3c-import.log:14
> +None

This changes seems unrelated and should be be part of the patch.

Created attachment 386418 [details]
Patch

Created attachment 386419 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 386419 [details]
Patch

Clearing flags on attachment: 386419

Committed r253922: <https://trac.webkit.org/changeset/253922>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58218115>