-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added initial set of tests for properties of CSS Borders 4 #42345
Added initial set of tests for properties of CSS Borders 4 #42345
Conversation
There are no reviewers for this pull request. Please reach out on the chat room to get help with this. Thank you! |
@fantasai @LeaVerou @Loirooriol That's the first time I am adding web platform tests. Maybe you could review them? (As I am not a contributor to this project yet, I am not allowed to add reviewers directly to this PR. That's why I wrote this comment instead.) Sebastian |
@SebastianZ Sebastian, I can do some review for you :) 1- As far as I can see, the lint integration check failed because " So, you must remove those "nasty" tab characters at the beginning of lines. " "
" " 2- 3- I recommend to embold no red, to use "visible on the page" is pleonastic, not useful, self-evident. In HTML5, the closing tag 4- In all fairness, this should be in a documentation file... 5- 6-
In your corner-shape-angle.html , I would say that your meta test text assert is not needed or not decisive. What you did is not a mistake. "
The test assertion is optional, but is highly recommended. It helps the reviewer understand the goal of the test so that he or she can make sure it is being tested correctly. Also, in case a problem is found with the test later, the testing method (e.g. using color to determine pass/fail) can be changed (e.g. to using background-color) while preserving the intent of the test (e.g. testing support for ID selectors). Examples of good test assertions:
" 7- " Same issue with border-clip-valid.html border-clip-invalid.html and maybe other tests. What you did is not necessarly a mistake but it is not ideal. Conclusion |
8- Eg
in border-radius-side-shorthands-001-ref.html Only the first 600px of height of screen shots are compared. The documentation says the viewport must be (maximum) 800px wide and 600px tall but even that is not clear. Elsewhere it is written 600px by 600px. Solution: smaller boxes or 2 rows of 2 boxes side-by-side or even split tests in 2 sub-tests 9- In the test |
Thank you @TalbotG for your review! I'll try to fix and improve all the things you mentioned over the next few days. As a start, I've addressed the linting issues. Sebastian |
At line 20 of the file |
In You have tested Do you intend to also test other mixed values like |
decf69d
to
42165bc
Compare
@TalbotG I think I've addressed all your feedback. Please let me know if there's anything missing!
That's the only thing I didn't change as basically all other CSS spec. tests have this *-valid.html, *-invalid.html, *-computed.html naming convention in their parsing/ subfolder, even the newest ones like css-view-transitions. Sebastian PS: I really don't like leaving out tags like |
Sebastian, 10- 11- 12- 13- Same thing with Also in |
14- |
Then fine. Leave these tests' filenames as they are. No problem.
I am like you. I prefer to edit |
Made the requested changes.
Sure, though I went with your general rule for making squares 100px wide to emphasize the borders.
Yes, done that not only for that test but als for the others with completely rounded corners.
Ok, removed the assertions from those tests and improved the sentences in the other tests. Sebastian |
One last modification... 15- Green and red colors should always be linked, should be hand-in-hand, should be mirrored. If you use green color in a test, then red color should be involved in the test. If a test's pass-fail conditions sentence state "(...) no red", then it must use the green color to indicate success. This is an established convention, more or less a standard now when doing CSS tests. It has been the case since the beginning of reftests over at W3C. " " Red So the 2 border-radius-side-shorthands tests and their related references should be updated accordingly. Just replace where needed "black" with "green". |
1d36f74
to
6657848
Compare
Makes sense. Done. Sebastian |
@SebastianZ Sebastian, "there are four black squares" I expect to be less available in the next 3 days. |
6657848
to
5c8012d
Compare
Oops! But it's ok for the black-green colorblind people.😆 Fixed it now.
No worries. There's no hurry. Sebastian |
@SebastianZ Sebastian, I have not forgotten you. I am in the middle of computer migration and personal matter that require my time and undivided attention. I will make a final review of your tests - now that all formatting issues that I could find have been all covered and corrected by you - later this week: this upcoming Thursday February 1st or Friday February 2nd. |
@TalbotG Thanks for the heads up and in advance for the review! Sebastian |
16- 17- More info: There may be a way to create reference files 18- 19-
20- 21- |
22- |
23- "The medium font-size computes to 16px." [Addendum] |
24- test_computed_value() function is called, is used in all border-*-computed.html tests in /parsing/ but I can not find test_computed_value() function declared in any of the .js files. In [Edit] http://wpt.live/css/support/computed-testcommon.js So you need to make appropriate changes in |
25- test_computed_value() when it should be test_valid_value() I have not checked other tests. |
@TalbotG Ok, I think I incorporated all your last suggestions. I also did a rebase and removed the file incorrect file movement commits completely, so we should be good on that. Sebastian |
f2990a8
to
b119144
Compare
@TalbotG I didn't see any further issues. Please let me know if I missed anything! @dbaron I did add some fuzziness to the tests now in 48b9f4a based on some educated guesses looking at the reference renderings, calculating the pixels drawn for anti-aliasing and adding some buffer. Sebastian |
Sebastian, commit b119144 failed... and, right now, I do not know why. " https://github.com/web-platform-tests/wpt/runs/25140115232 The Azure Pipeline checks got queued and never ended... At line 8 of Regarding fuzziness matching in tests and proper values for maxDifference and totalPixels. The proper sequence of events should be: 1- Browser manufacturers implement new property with its correspondent property values In your case, no browser manufacturer has so far implemented any of the property values. So, it is premature to come up with maxDifference and totalPixels values. I am still looking, searching why |
Sebastian, Regarding why commit b119144 failed, |
@SebastianZ Sebastian, @whimboo Henrik says in this comment in Issue 46378 that we need to close the Pull Request and reopen it in order to trigger the Continuous Integration checks, especially for commit b119144 which obviously has problems (" Some checks were not successful So I am going to do it now... [fingers crossed] |
Closing PR now |
Reopening PR now |
Yeah, I agree the pixel counts for those fuzziness values look surprisingly large; I agree that it's better not to include them for now, though it might be nice to leave a comment that says that you know the test will eventually need a meta name=fuzzy, but you just don't know what the required numbers are. |
@dbaron I tried my best to estimate the required fuzziness by counting pixels, though I guess my buffer of two times the value was too big. Anyway, I've done now as suggested and replaced the @TalbotG Excuse my long silence on this again! According to @foolip it sounds like the failing commit doesn't matter because the commits will be squashed when merging them. Though let's see if that works. If not, I can close this PR and create a new one with the changes. Sebastian |
…orner-shape tests Instead of adding <meta name="fuzzy"> tags with guessed values, there's now a todo comment indicating that the tests need to be updated to use fuzzy matching.
849e719
to
5f58b8e
Compare
I am ready to approve all this (72 conversations, 59 files, 19 commits, many corrections, 5½ months long). [I am opening a bottle of champagne right now :) ] From now on, Sebastian, please try to create smaller pull requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving.
@SebastianZ Sebastian, All of the corner-shape tests results should eventually be listed in a corner-shape folder inside https://wpt.fyi/results/css/css-borders/tentative and, right now, they are not. The corner-shape tests are in: http://wpt.live/css/css-borders/tentative/corner-shape/ and their associated reference files are in: http://wpt.live/css/css-borders/reference/ So far, after some random visual check on my part, everything seems correct (including the removal of |
Thank you again for all your efforts and help, @TalbotG! It was very much appreciated! And I learned a lot from all this!
Cheers!😄🍾
Promise! Sebastian |
I'll observe that. Sebastian |
All of the corner-shape tests results are now listed in |
In https://wpt.fyi/results/css/css-borders/tentative/corner-shape/corner-shape-round-top-left-001.html When you can, create a Pull request to remove that "e" and, at the same time, remove and then add me as a reviewer to such Pull request. |
This adds an initial set of tests related to the CSS properties introduced in CSS Borders 4.
Sebastian