web-platform-tests
Added initial set of tests for properties of CSS Borders 4
This adds an initial set of tests related to the CSS properties introduced in CSS Borders 4.
Sebastian
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- The lint integration check failed. In other words, this is an important task and an important integration check to pass. You must pass such test before "earning" the right for a review from reviewers :)
As far as I can see, the lint integration check failed because
" Test-file line starts with one or more tab characters (INDENT TABS) "
So, you must remove those "nasty" tab characters at the beginning of lines.
" line 951: INFO:lint:There were 842 errors (INDENT TABS: 842) " coming from the Lint log
" INDENT TABS
Test-file line starts with one or more tab characters
To fix:
use spaces to replace any tab characters at
beginning of lines
" coming from http://web-platform-tests.org/writing-tests/lint-tool.html
" Use spaces rather than tabs for indentation " coming from http://web-platform-tests.org/writing-tests/general-guidelines.html#style-rules
2-
<meta charset="utf-8" />
There is no need for the Null End Tag everywhere in your tests. It is not a mistake; it is just not needed, not necessary.
<meta charset="utf-8">
is good.
3-
<p>Test passes if there are four black boxes with rounded corners and red is not visible on the page.</p>
I recommend to embold no red, to use <strong>no red</strong> so that this pass conditions stands out.
"visible on the page" is pleonastic, not useful, self-evident. That's item 12 in my list in https://lists.w3.org/Archives/Public/public-css-testsuite/2018Jun/0000.html " Why remove such words? Because they are useless, empty-meaning, pleonastic or self-evident or irrelevant words; they can be safely removed without affecting understanding of pass/fail conditions. Anything that shortens the pass/fail conditions sentence of a test, without compromising its understanding for a tester, helps. "
In HTML5, the closing tag </p> can be removed too. I have been personally requested to remove all optional closing tags from my tests.
Even <head> , <body> and <html> are optional and so I removed them all from my tests: I have been requested to do so.
4- New line missing at end of file of many of your tests. You need to add an empty line at end of file of your tests. If it is missing, then you will see an icon of a red circle with minus sign inside of it when the reviewer views your tests in the Commits tab .
In all fairness, this should be in a documentation file...
5-
For title text, I recommend to reuse the important description words of the spec like this eg:
<title>CSS Borders and Box Decorations Test: Logical 'border-radius-*' shorthands set the two radii of one side.</title>
instead of
<title>CSS Test: Logical 'border-radius-*' shorthands set the two radii of one side.</title>
6- Meta test text assert. This is optional. If the test is complex or involving some particular code scenario, then you may need to write some explanation with the goal of helping the reviewer and the eventual testers. I recommend to start with the following
<meta name="assert" content="This test checks that ...">
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 assertion should not be:
A copy of the title text
A copy of the test verification instructions
A duplicate of another assertion in the test suite
A line or reference from the CSS specification
unless that line is a complete assertion when
taken out of context.
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:
“This test checks that a background image with
no intrinsic size covers the entire padding box.”
“This test checks that ‘word-spacing’ affects each
space (U+0020) and non-breaking space (U+00A0).”
“This test checks that if ‘top’ and ‘bottom’ offsets are
specified on an absolutely-positioned replaced element,
then any remaining space is split amongst the ‘auto’
vertical margins.”
“This test checks that ‘text-indent’ affects only the
first line of a block container if that line is also the
first formatted line of an element.”
" coming from http://web-platform-tests.org/writing-tests/css-metadata.html#test-assertions
7- Filenaming. corner-shape/corner-shape-round.html It is best to end with a number, like, eg, 001 in filenames. So that if you or someone else want to write another test involving a rounded corner, then such person only has to continue from the last number in the filename.
" A common format is test-topic-001.html, where test-topic is a short identifier that describes the test. It should avoid conjunctions, articles, and prepositions as it should be as concise as possible. The integer that follows is normally just increased incrementally, and padded to three digits. (If you’d end up with more than 999 tests, your test-topic is probably too broad!) " coming from http://web-platform-tests.org/writing-tests/general-guidelines.html#file-paths-and-names
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 the most important things for you right now is to correct the 842 tab characters (INDENT TABS) and add an empty line at EOF of several of your tests.
8- Some (several?) of your tests exceed the 600px height window viewport constraint. " The test renders within a 800x600 viewport " coming from http://web-platform-tests.org/reviewing-tests/checklist.html#visual-tests-only
Eg
<div id="reference1"></div>
<div id="reference2"></div>
<div id="reference3"></div>
<div id="reference4"></div>
in border-radius-side-shorthands-001-ref.html where each div is 200px tall with a vertical margin of 10px ~= 986px.
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 border-radius-side-shorthands-001-ref.html the borders of the 4 boxes should be black according to pass-fail condition sentence but their borders are definitely red because the code says so!
In the test border-radius-side-shorthands-001.html it is not possible for the test to display red. So the pass-fail condition sentence is not right, is not consequent.
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
@SebastianZ
At line 20 of the file
box-shadow-offset-computed.html
I see:
test_computed_value("box-shadow-offset", "1F0px 20px, 30px 40px");
The "F" should not be there.
In corner-shape-mixed-values-001.html you are testing 'corner-shape: round angle' which is 2 values while the text assert says "Three different values are supported for 'corner-shape'"
You have tested 'corner-shape: angle' 'corner-shape: round angle' 'corner-shape: round angle round' 'corner-shape: round angle round angle' 'corner-shape: round'
Do you intend to also test other mixed values like 'corner-shape: angle round' 'corner-shape: angle round angle' 'corner-shape: angle round angle round' and 'corner-shape: round angle angle angle' 'corner-shape: round angle angle round' 'corner-shape: round round angle angle' etc. ?
@TalbotG I think I've addressed all your feedback. Please let me know if there's anything missing!
7- Filenaming. ... 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.
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 <html>, </p> and prefer a more XML-like syntax, though as most of the tests seem to do that I followed your advice and adjusted the structure accordingly.
Sebastian,
10- You can safely remove the words "on the page" from the pass-fail conditions sentence everywhere. Since 95% (or so) of all tests starts with a sentence stating the pass and fail conditions, then all these expressions: "in this page", "on this page", "on the screen", "below", "below this line", "after this line", "after this sentence", "below this paragraph", "under this paragraph", "after this", "following", "which follows", etc.. are evident, are pleonastic, are not useful, not needed.
11-
Only the 2 border-radius-side-shorthands tests needed to have shorter div because it seemed that only those 2 were creating taller than 600px document box. What you did (reducing all the divs from 200px to 100px everywhere) is not a mistake.
12- For the test corner-shape-round-001.html I suspect it is possible to create a reference for it... Do you agree?
13-
In box-shadow-blur-computed.html tests
you can safely remove
line 6: <meta name="assert" content="Computed value of 'box-shadow-blur' is correct">
because such test assert text is not helpful, not meaningful, not worthy, not significant.
Same thing with
<meta name="assert" content="Computed value of 'box-shadow-color' is correct">
in box-shadow-color-computed.html
Also in box-shadow-offset-computed.html , box-shadow-position-computed.html , box-shadow-spread-computed.html and corner-shape-computed.html
14- Reference files should go in (move to) a /reference folder: this is best. What you did is not a mistake. But it is better to have such folder separation.
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.
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.
Then fine. Leave these tests' filenames as they are. No problem.
Sebastian
PS: I really don't like leaving out tags like
<html>,</p>and prefer a more XML-like syntax
I am like you. I prefer to edit <html>, <body>, </p> and all optional closing tags (</tr>, </td>, etc.) but I was told and requested to make that change. If you visit my website, you will see that I use HTML4.01 strict almost everywhere.
Made the requested changes.
11- Only the 2 border-radius-side-shorthands tests needed to have shorter
divbecause it seemed that only those 2 were creating taller than 600px document box. What you did (reducing all the divs from 200px to 100px everywhere) is not a mistake.
Sure, though I went with your general rule for making squares 100px wide to emphasize the borders.
12- For the test corner-shape-round-001.html I suspect it is possible to create a reference for it... Do you agree?
Yes, done that not only for that test but als for the others with completely rounded corners.
13- In box-shadow-blur-computed.html tests you can safely remove line 6:
<meta name="assert" content="Computed value of 'box-shadow-blur' is correct">because such test assert text is not helpful, not meaningful, not worthy, not significant. ...
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 Means Failure Don't use the color red other than to indicate a failure. (Exception: testing for support of red) Since green-with-no-red is often used to indicate success, it's best to also avoid green unless using the presence of red to indicate failures. " coming from https://wiki.csswg.org/test/format#design-requirements
" In general, using colors in a consistent manner is recommend. Specifically, the following convention has been developed:
Red Any red indicates failure. Green In the absence of any red, green indicates success. " coming from 2003's CSS2.1 Test Case Authoring Guidelines document https://www.w3.org/Style/CSS/Test/guidelines.html#color
So the 2 border-radius-side-shorthands tests and their related references should be updated accordingly. Just replace where needed "black" with "green".
15- Green and red colors should always be linked ... So the 2 border-radius-side-shorthands tests and their related references should be updated accordingly. Just replace where needed "black" with "green".
Makes sense. Done.
Sebastian
@SebastianZ Sebastian,
"there are four black squares" must become "there are four green squares" in the 2 border-radius-side-shorthands tests and in their related references.
I expect to be less available in the next 3 days.
@SebastianZ Sebastian,
"there are four black squares" must become "there are four green squares" in the 2 border-radius-side-shorthands tests and in their related references.
Oops! But it's ok for the black-green colorblind people.😆 Fixed it now.
I expect to be less available in the next 3 days.
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-
In border-radius-side-shorthands-001.html
I see
line 77: border-radius-right: 1 4em / 5em 1em;
The "1" should be "2em" .
17- In all of corner-shape-four-values tests (except 001): if there is no reference file for those tests, then you must append "-manual" to such tests: "a -manual suffix after the main filename but before the extension".
More info: http://web-platform-tests.org/writing-tests/manual.html
There may be a way to create reference files for each and all corner-shape-* tests but, right now, I do not know.
18- In corner-shape-four-values-001 meta name="assert" content="This test checks that 'round round round round' is supported for 'corner-shape'." but, in reality, the test is currently passed by Firefox and Chromium because of a fallback feature: if a property (and/or a property value) is not implemented, then it is ignored. This is why the corner-shape-four-values-001 test apparently and seemingly passes right now in Firefox and Chromium (and also - I have not checked - most likely pass in Safari) but corner-shape is not implemented. So, this particular test assert text in corner-shape-four-values-001 is not true, is not correct. I think now you need to remove that corner-shape-four-values-001 test after all... [Addendum] Same thing with the tests corner-shape-one-value-001.html and corner-shape-three-values-001.html and corner-shape-two-values-001.html [/Addendum]
19- In tests and/or in references, if you can combine selectors, then this is preferable. Eg In border-radius-side-shorthands-001-ref.html in line 20 to line 46, the code can become:
#reference-shape-1 , #reference-shape-3 {
border-top-left-radius: 40px;
border-top-right-radius: 40px;
border-bottom-right-radius: 40px;
border-bottom-left-radius: 40px;
}
#reference-shape-2 , #reference-shape-4 {
border-top-left-radius: 3em 1em;
border-top-right-radius: 2em 4em;
border-bottom-right-radius: 5em 1em;
border-bottom-left-radius: 2em 4em;
}
20- Right now, I think your pull request is considerably too big (83 files). Can you put all of the parsing tests (computed, valid, invalid) into a distinct and separate pull request? I may be able to approve those parsing tests. And then I will be able to concentrate on your other remaining tests and references.
21- One way to help reviewers is to put all of your tests and references into an accessible website. Eg http://www.gtalbot.org/BrowserBugsSection/CSS3Backgrounds/ http://www.gtalbot.org/BrowserBugsSection/CSS3WritingModes/ That way, the reviewer can click and load the tests and references into a browser to see+examine the code in the files. Otherwise, the reviewer has to do a lot of clicking, loading into a text editor, file creating, uploading onto a website or local loading on his/her computer. I have started to do this with your tests and references: http://www.gtalbot.org/BrowserBugsSection/CSS4BordersBoxDecorations/ I have appended "-SZ" to your tests and references.
22- The references border-radius-side-shorthands-001-ref.html and border-radius-side-shorthands-002-ref.html are identical. So, best is to remove -002 and reused -001 accordingly.
23- In parsing/border-clip-computed.html at line 20, I see test_computed_value("border-clip", "10px 1em 10% 1fr 2fr", "10px 20px 10% 1fr 2fr"); this implies that initial, default font size of body text is assumed to be 20px in the test. The problem here is that initial, default font size for unstyled body text in CSS tests is supposed to be 16px. 16px is the default de facto web browser industry font size for unstyled body text.
"The medium font-size computes to 16px." coming from http://web-platform-tests.org/writing-tests/assumptions.html
[Addendum] Same thing in parsing/box-shadow-blur-computed.html at line 15: test_computed_value("box-shadow-blur", "1em", "20px"); and in parsing/box-shadow-spread-computed.html at line 15: test_computed_value("box-shadow-spread", "1em", "20px"); and at line 21: test_computed_value("box-shadow-spread", "calc(1em + 1px)", "21px"); and in parsing/corners-computed.html at several lines (lines 21, 22, 26, 27, 29, 32, 33 and 48), 1em is assumed to compute to 20px [/Addendum]
24- Where (in which .js file) is declared the test_computed_value() function?
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 http://wpt.live/css/support/parsing-testcommon.js we can see test_valid_value() and test_invalid_value() but not test_computed_value()
[Edit] I found it. It is in
http://wpt.live/css/support/computed-testcommon.js
So you need to make appropriate changes in all of your parsing/border-*-computed.html tests. [/Edit]
25- In parsing/border-clip-valid.html the function called in line 13 to line 19 is
test_computed_value()
when it should be
test_valid_value()
I have not checked other tests.
Sebastian, I expect to be less available in the next upcoming 4-5 days. I will get back to you on Friday February 9th or Saturday February 10th.
26-
Several of your parsing tests are not supported by the current spec.
This section https://drafts.csswg.org/css-borders-4/#index-defined-here lists each individual property names covered by the spec. 'border-radius-*' do not exist (eg.border-radius-bottom , border-radius-top , border-radius-left, etc.: these are not listed in the spec). So, the problem is how (or why) can you be testing, for example, 'border-radius-bottom' in the filename border-radius-bottom-valid.html ? Same thing with several other tests in /parsing/ .
There is no problem with testing 'border-bottom-radius' though.
Thanks again, @TalbotG, for the review and all the detailed feedback! I am still working on fixing all the things you mentioned and hope to finish them tomorrow, Saturday February 10th.
Sebastian
Sebastian,
There is now a lot of work involved in your tests and overall they look much better.
3 quick comments.
a) I do not know, I can not explain now why commit edb625f failed.
b) parsing/border-radius-inline-end-invalid.html was filename-renamed as parsing/border-inline-end-radius-invalid.html ; it was only filename-renamed; the code inside was not updated accordingly.
c) In my opinion, meta assert text should be edited only if a test is complex, involves a particular code scenario. If a test is simple or testing something basic, then there is no need for a test assert text.
16- In border-radius-side-shorthands-001.html I see line 77:
border-radius-right: 1 4em / 5em 1em;The "1" should be "2em" .
Fixed and reworked the tests.
17- In all of corner-shape-four-values tests (except 001): if there is no reference file for those tests, then you must append "-manual" to such tests: "a -manual suffix after the main filename but before the extension".
I've now added reference files for all the tests using SVGs to show the expected display.
18- In corner-shape-four-values-001 meta name="assert" content="This test checks that 'round round round round' is supported for 'corner-shape'." but, in reality, the test is currently passed by Firefox and Chromium because of a fallback feature: ...
I've removed those tests.
19- In tests and/or in references, if you can combine selectors, then this is preferable. Eg In border-radius-side-shorthands-001-ref.html in line 20 to line 46, the code can become:
Done. (Though I've also reworked some of the tests to improve verifiability, so some selectors could not be combined anymore.)
20- Right now, I think your pull request is considerably too big (83 files). Can you put all of the parsing tests (computed, valid, invalid) into a distinct and separate pull request? I may be able to approve those parsing tests. And then I will be able to concentrate on your other remaining tests and references.
Fair point. I've split the parsing tests into #44525.
21- One way to help reviewers is to put all of your tests and references into an accessible website.
I don't have my own website. So I'll need to check where to upload those, maybe GitHub.io. For now, you'll have to upload them on your site to check them or run a local server, sorry!
22- The references border-radius-side-shorthands-001-ref.html and border-radius-side-shorthands-002-ref.html are identical. So, best is to remove -002 and reused -001 accordingly.
Done.
23- In parsing/border-clip-computed.html at line 20, I see test_computed_value("border-clip", "10px 1em 10% 1fr 2fr", "10px 20px 10% 1fr 2fr"); this implies that initial, default font size of body text is assumed to be 20px in the test. The problem here is that initial, default font size for unstyled body text in CSS tests is supposed to be 16px. ...
Not sure why I assumed 20px. I think I initially had set font-size: 20px; on the element. I changed the expected values to 16px now.
24- Where (in which .js file) is declared the test_computed_value() function? ...
So you need to make appropriate changes in all of your parsing/border-*-computed.html tests. [/Edit]
Fixed.
25- In parsing/border-clip-valid.html the function called in line 13 to line 19 is
test_computed_value()
when it should be
test_valid_value()
I have not checked other tests.
Copy & paste error. Thanks for noticing! Fixed.
26-
Several of your parsing tests are not supported by the current spec.
This section https://drafts.csswg.org/css-borders-4/#index-defined-here lists each individual property names covered by the spec. 'border-radius-*' do not exist (eg.border-radius-bottom , border-radius-top , border-radius-left, etc.: these are not listed in the spec). So, the problem is how (or why) can you be testing, for example, 'border-radius-bottom' in the filename border-radius-bottom-valid.html ? Same thing with several other tests in /parsing/ .
They were initially defined as border-radius-* but later got renamed to border-*-radius for consistency with the existing border-*-radius properties. I've adjusted the tests now to fit to the names defined in the spec.
a) I do not know, I can not explain now why commit edb625f failed.
I was still in the middle of reworking the tests. It looks like they passed now.
b) parsing/border-radius-inline-end-invalid.html was filename-renamed as parsing/border-inline-end-radius-invalid.html ; it was only filename-renamed; the code inside was not updated accordingly.
Same as above. Please check #44525.
c) In my opinion, meta assert text should be edited only if a test is complex, involves a particular code scenario. If a test is simple or testing something basic, then there is no need for a test assert text.
I thought I'd better add one to all the tests to let testers know what's expected. Though let me know if I should rather remove them.
Sebastian
put all of your tests and references into an accessible website. (...) you'll have to upload them on your site to check them
I can do that. I will do that. No problem.
put all of your tests and references into an accessible website. (...) you'll have to upload them on your site to check them
Done http://www.gtalbot.org/BrowserBugsSection/CSS4BordersBoxDecorations/
2 border-radius-side-shorthands files 26 /corner-shape files 4 .js files 16 /reference files 48 /parsing files 15 /support files = = = = 111 files
plus 2 of mine
GT-corner-shape-four-values-004-GT.html and
GT-corner-shape-angle-bottom-left-ref-GT.html
(I'll explain why these 2 later)
Sebastian,
I am afraid that you will have to filename-rename all your corner-shape-* tests. When creating a reference file, you must reuse the filename of the associated test.
If the test's filename is, e.g., dkwidc-001.html then its associated reference must be dkwidc-001-ref.html
Here, corner-shape-[ four | three | two | one ]-values.html is not as descriptive as
corner-shape-[angle | round]-*-ref.html
and your .svg filenames would be matching such corner-shape-[angle | round]-* filename.