Skip to content
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

The wpt Writing Tests documentation should be updated #45943

Open
TalbotG opened this issue Apr 27, 2024 · 4 comments
Open

The wpt Writing Tests documentation should be updated #45943

TalbotG opened this issue Apr 27, 2024 · 4 comments
Labels

Comments

@TalbotG
Copy link
Contributor

TalbotG commented Apr 27, 2024

After submitting many CSS tests and after reviewing a bunch of them, I am convinced that the
Writing Tests documentation
should be updated in numerous spots and areas. Here is the start of a list of items that should become part of the Writing Tests documentation. I welcome others to add items to this short list.

1-
No new line at end of file. If a new line character is missing at end of file, then an icon of a red circle with a red minus sign inside of it is visible in the Commits tab. Ideally, someone could explain why the new line character at end of file is important.

And maybe the lint tool should report missing new line character at end of CSS tests files too.

2-
How to restart the Continuous Integration checks of a Pull Request? By closing the Pull Request and reopening it. This should be documented somewhere in the
Writing Tests documentation
as a tip.

3-
Null End Tag. They are not a mistake but they are not needed, not useful, not necessary. They occupy, use 2 bytes each for no reason since CSS tests resort to and require HTML5.

4-
800x600 or 600x600 viewport. The documentation should choose and clarify this.

"
The test renders within a 800x600 viewport
"
coming from
http://web-platform-tests.org/reviewing-tests/checklist.html#visual-tests-only

while elsewhere it is written 600x600

5-
Optional end tags. </p>, </li>, </dt>, </option>, </tr>, </td>, </body>, </head>, </html>, etc... are not necessary, not needed and not useful for CSS testing.
Same thing with optional start tags like <html>, <head>, <body> . The documentation should formally promote the absence of optional HTML tags in CSS tests.
HTML5, §13.1.2.4 Optional tags

6-
Larger line spacing of the documentation itself. By setting line-height: 1.5 to the body node, the Writing Tests documentation itself would be easier to read. The documentation itself should strive to become compliant with WCAG 2 AAA, § 1.4.8 Visual Presentation:
"Line spacing (leading) is at least space-and-a-half within paragraphs, and paragraph spacing is at least 1.5 times larger than the line spacing."

7-
Length of tests filenames. Some tests' filenames are as long as 65 characters. I have not searched extensively (albeit PR43587) but maybe there are some filenames even longer than 65 characters. I suggest a 32-characters long filename as ideal and I would set the limit to 48-characters long. A filename should not substitute or should not try to substitute to a title text, text assert (if and when suitable) and to shepherding comments inside a test.

8-
Where to get maxDifference and totalPixels values. Somewhere inside the Fuzzy Matching section, the documentation could indicate how to get those values.

@gsnedders , @SebastianZ , @dbaron , @fantasai , @frivoal

@TalbotG TalbotG added the docs label Apr 27, 2024
@fantasai
Copy link
Contributor

fantasai commented May 8, 2024

@gsnedders Where does the web-platform-tests.org documentation even live?

@dbaron
Copy link
Member

dbaron commented May 8, 2024

A few comments:

  • I think in general if a rule doesn't add significant value, we shouldn't make it. More rules to enforce are more rules to read, learn, and understand, and this reduces the chance that people will follow other more important rules. In particular, this makes me want to avoid making more rules such as (5) and (7). That said, I think there are some real file length limits that WPT users face, but I think they probably mostly relate to the full path length and not the length of any component of it.

  • Regarding point 4: The 600x600 comes from a CSS WG resolution.

  • Regarding point 8 (although I'm not sure if you were asking): maxDifference and totalPixels values are available from many test runners, including from wpt.fyi, and including the wpt.fyi links in the Checks tab of a PR. (Just because they can be found doesn't necessarily mean that the test should permit that amount of fuzziness, though.)

  • Regarding point 3: "null end tag" is an SGML feature that allows syntax like <title/Document Title/ as a shorthand for <title>Document Title</title>. It is not part of the HTML parsing algorithm and I would be surprised if it is present in WPT tests. Are you talking about empty-element TAGS (an XML feature)?

  • Regarding points 3 and 5: I think in many cases it's good for tests to vary across the normal ways that web content varies; this sometimes allows them to catch additional issues that they wouldn't otherwise have caught. I'd like to avoid being too prescriptive when it doesn't add value. And specifically on point 5 I don't want to encourage lots of edits to existing tests for stylistic reasons, which risks breaking the tests unnecessarily; sometimes "optional" tags matter, such as when a <body> is followed by a <script>...</script> and the script needs to run inside the body rather than the head.

@TalbotG
Copy link
Contributor Author

TalbotG commented May 11, 2024

David,

Thank you for taking the time to write your detailed comments.

More rules to enforce are more rules to read, learn, and understand, and this reduces the chance that people will follow other more important rules.

We have review of tests and there are test reviewers. Professionalisation of tests and testing insure quality of tests and testing. I have never rejected tests unless they were false, bad or fundamentally incorrect. I always have tried to rehabilitate inadequately (or poorly) formated tests as long as they were testing a testable spec statement to begin with.

point 3: The null end tag I was referring to is, e.g.:
<meta charset="utf-8" />
when
<meta charset="utf-8">
is already good, already functional and sufficient.

point 5 (Optional end tags): I have been formally asked to remove all optional end tags and optional tags (<html>, <head>, <body>) from my tests. And so, I thought the wpt Writing Tests documentation should state such requirement.

If CSS tests do not need optional end tags and optional tags and if minimal - reduced as much as possible - filesize is a recommandable goal in itself and by itself, then such requirement makes sense. As I understand, it takes several hours to several computers to go through all the wpt tests for the 4 browsers. Only necessary filesize makes sense in that regard.

Varying HTML content (tags and elements) makes sense for HTML testing, when testing HTML.

point 7 (Length of tests filenames): Only a very small minority of CSS tests exceed 60 characters. I wish I knew how to find them (with a grep command) and list them in order to establish precisely their percentage from the whole set of CSS tests. I estimate (just a hunch) that less than 0.1% use, exceed more than 60 characters.

A filename should not be a sentence describing a test. A long (> 48 characters) filename does not help review, does not replace a title text and does not replace useful comments or a text assert. I do not see how anyone gets help from a long filename.
A long filename will trigger horizontal overflow, horizontal scrolling (and scrollbar) in various situations (tables) or webpages and/or creates ellipsis ("…") characters in various places. If a long filename creates ellipsis ("…") characters, then we don't even get to read such filename anyway.
My proposal is that, from now on, the lint checking tool detects and reports a filename exceeding 48 characters and asks the author to limit to 48 characters or less.

point 8 (Where to get maxDifference and totalPixels values.)

maxDifference and totalPixels values are available from many test runners

I never found such information all by myself. And recently, Sebastian Zartner was wondering and asking where he could find those values.

Just because they can be found doesn't necessarily mean that the test should permit that amount of fuzziness, though.

Of course. Agreed.


I will be away from web and internet for a week.

@TalbotG
Copy link
Contributor Author

TalbotG commented May 20, 2024

on point 5 I don't want to encourage lots of edits to existing tests for stylistic reasons, which risks breaking the tests unnecessarily

First, my proposal wrt point 5 was not for existing CSS tests but for upcoming CSS tests.
Second, my proposal is not for stylistic reasons, is not about stylistic reasons.
Third, even if we would remove </p>, </li>, </dt>, </option>, </tr>, </td>, </body>, </head>, </html> from existing CSS tests, - which is not my goal on point 5 - , why would CSS tests break? I do not see how or why CSS tests could or would break. All these are optional tags, even in HTML4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants