RE: TestTWF Pull requests needing review

> From: "Gérard Talbot" [mailto:css21testsuite@gtalbot.org]
> Sent: Friday, November 8, 2013 4:18 AM
> To: Zhang, Zhiqiang
> Cc: Rebecca Hauck; Public CSS testsuite mailing list
> Subject: RE: TestTWF Pull requests needing review
> 
> 
> Le Mer 6 novembre 2013 21:46, Zhang, Zhiqiang a écrit :
> > From: Rebecca Hauck [mailto:rhauck@adobe.com]
> > Sent: Tuesday, October 29, 2013 6:49 AM
> >
> > CSS UI:
> > -------
> > https://github.com/w3c/csswg-test/pull/95
> > https://github.com/w3c/csswg-test/pull/97
> >
> > I've reviewed the 2 PRs on GitHub and made an improvement:
> >
> > http://zqzhang.github.io/review/box-sizing-001-review.html
> > http://zqzhang.github.io/review/box-sizing-002-review.html
> > http://zqzhang.github.io/review/box-sizing-001-ref-review.html
> >
> > Any comment is welcome.
> 
> [Resending this email as my webhost SMTP server seemed to have failed
> sending it.]
> 
> Zhiqiang,
> 
> Here's some feedback on these tests.
> 
> http://zqzhang.github.io/review/box-sizing-001-review.html
> 
> line 4:
> <title>CSS Basic User Interface Test: box-sizing - padding-box</title>
> 
> Suggestion:
> <title>CSS Basic User Interface Test: box-sizing - padding-box
> (basic)</title>

Better, updated.

> 
> or
> 
> <title>CSS Basic User Interface Test: box-sizing - padding-box
> (simple)</title>
> 
> 
> line 6: <link rel="help" href="http://www.w3.org/TR/css3-ui/#box-sizing">
> 
> Suggestion:
> <link rel="help" href="http://www.w3.org/TR/css3-ui/#box-sizing"
> title="6.1. 'box-sizing' property">

Added. Though title is not suggested by reftest template.

http://testthewebforward.org/docs/test-templates.html#reftest-test-case-template-

> 
> 
> line 11 to 17:
>         div {
>             height: 200px;
>             left: 10px;
>             position: absolute;
>             top: 50px;
>             width: 200px;
>         }
> 
> setting left and top to an arbitrary value is not needed by the test and
> not necessary in the test.

Yes, agree. I thought to keep the original test code.

> 
> We already have a reftest which is very frequently used and reused for
> tests which would fit perfectly here:
> 
> http://test.csswg.org/source/approved/css2.1/src/reference/ref-filled-
> green-100px-square.xht
> 
> Shepherd indicates that such reftest is referenced by 151 tests already
> 
> http://test.csswg.org/shepherd/reference/ref-filled-green-100px-square/
> 
> So, I would change 200px (height and width) for 100px and then just
> reuse that reftest. In the long term, reducing the number of reftests
> brings many benefits: reusability, reduce intricability (N tests to 1
> reftests instead of multiple 1 test to 1 reftest), more efficient memory
> management.
> 
> 
> line 21:
>             padding: 20px;
> 
> In other to maximize noticeability of test failure, I would set padding
> to 50px.

This is very good, updated.

> 
> 
> 
> line 30-31:
>     <div class="test-overlapped-red"></div>
>     <div class="ref-overlapping-green"></div>
> 
> Semantically speaking, class is for logical grouping of several
> elements. Id is for linking to a document-unique element. So, here, I
> would use id instead of class.

Agree.

Gérard, since you and I have reviewed the tests, I added you and me as reviewers to them. Thank you.

hayashih and kasshisatari, feel free to upload the tests.

Thanks,
Zhiqiang

Received on Friday, 8 November 2013 06:50:19 UTC