search

web-platform-tests / wpt

Test suites for Web platform specs — including WHATWG, W3C, and others
https://web-platform-tests.org/
Other
4.96k stars 3.08k forks source link

"Common" files are untested and poorly documented #17913

Open jugglinmike opened 5 years ago

jugglinmike commented 5 years ago

The files in the common/ directory are intended to be used by tests across all of WPT. This makes documenting their usage and verifying their correctness very important. Today, they are inconsistently documented and uniformly untested. Some are unused, and others are used so sparingly as to not justify their presence in common/.

Here's what I think we should do about that:

/cc @zqzhang @deniak @gsnedders (the current suggested reviewers for common/)

Documentation references to common/ directory - `README.md`: > Various resources that tests depend on are in `common`, `images`, and > `fonts`. - `docs/writing-tests/general-guidelines.md`: > Various support files are available in in the `/common/` and `/media/` > directories (web-platform-tests) and `/support/` (in css/). Reusing > existing resources is encouraged where possible, as is adding > generally useful files to these common areas rather than to specific > test suites. - `docs/writing-tests/testharness.md` > There are two utility scripts in that work well together with variants, > `/common/subset-tests.js` and `/common/subset-tests-by-key.js`, where > a test that would otherwise have too many tests to be useful can be > split up in ranges of subtests. For example:
Reference count for each file in common/ $ for f in $(ls common/ | grep -v headers); do echo $(git grep "common/$f" | wc -l) common/$f; done | sort -n 0 common/form-submission.py 0 common/META.yml 0 common/sleep.py 1 common/canvas-frame.css 1 common/canvas-index.css 1 common/canvas-spec.css 1 common/css-red.txt 1 common/echo.py 1 common/entities.json 2 common/arrays.js 2 common/object-association.js 3 common/stringifiers.js 4 common/dummy.xml 5 common/redirect-opt-in.py 6 common/domain-setter.sub.html 6 common/namespaces.js 6 common/subset-tests-by-key.js 8 common/dummy.xhtml 8 common/text-plain.txt 10 common/test-setting-immutable-prototype.js 15 common/slow.py 17 common/PrefixedPostMessage.js 28 common/PrefixedLocalStorage.js 31 common/performance-timeline-utils.js 57 common/redirect.py 142 common/media.js 143 common/subset-tests.js 183 common/worklet-reftest.js 191 common/blank.html 204 common/utils.js 390 common/get-host-info.sub.js 488 common/reftest-wait.js 902 common/canvas-tests.css 2209 common/security-features 2482 common/canvas-tests.js
Hexcles commented 5 years ago

Related #11256

Hexcles commented 5 years ago
  • define a convention for documenting each file
  • document each file
  • extend the documentation website build process to consume that pattern and generate a page for the common code

Agreed. We can probably just use JSDoc.

  • extend the lint tool to require documentation

This is nice to have but optional -- we can set CODEOWNERS for common/ and ask them to enforce/promote good documentation practices during review.

  • write tests for the code (likely stored in a new subdirectory of infrastructure/)

Agreed. Putting tests in infrastructure/ using testharness.js is good and we won't need any infra changes. I do not want to have yet another semi-independent suite like resources/test.

jugglinmike commented 5 years ago

Agreed. We can probably just use JSDoc.

I'd like to avoid inventing something new, as well, but since we have to support three programming languages, it might be an amalgamation of a few different conventions.

I do not want to have yet another semi-independent suite like resources/test.

I would love to fix this. We should talk about prioritizing it!

foolip commented 5 years ago

FWIW, what I've struggled the most with is understanding what things belong in common/ and which belong in resources/. In particular, check-layout-th.js and sriharness.js in resources/ seem like they'd fit as well in common/.

I haven't found that documentation of the files themselves are a problem once I know about them, for example I've used common/media.js a lot and it's quite self-explanatory.

jugglinmike commented 5 years ago

@foolip We should find out if common/ could be merged in to resources/

Ms2ger commented 5 years ago

I suspect the distinction between common and resources is historical; resources used to be a submodule (and was shared with the CSSWG test suite, IIRC). I don't see any obvious reasons not to merge them.

gsnedders commented 5 years ago

I think later the intent had been for resources to be things directly maintained as project infra, common to be everything else (maintained by the users of it).

jugglinmike commented 5 years ago

Is that still the intent today?

gsnedders commented 5 years ago

At least in my head that's still the distinction; whether the split between the two directories is correct for that is another question!

jugglinmike commented 5 years ago

My feeling is that we'll get more consistent documentation and test coverage if we designate a single group as the owners of the shared code.

jgraham commented 5 years ago

I wonder if the Chromium sync differentiates here? I know they don't update all the infrastructre every time it changes in GitHub.

Generally I have the same understanding of the distinction as @gsnedders; resources/ is project infrastructure, common/ is useful helper utilities that aren't essential infrastructure. I fully agree that some of the things that have subsequently been added to resources don't meet that bar and argubaly should be in common. But I don't have very strong feelings here.

Hexcles commented 5 years ago

I wonder if the Chromium sync differentiates here? I know they don't update all the infrastructre every time it changes in GitHub.

Are you referring to code ownership? If we decide to review resources/ specially, then I can add rules in Chromium accordingly (to require review from our team); so please keep me in the loop (FWIW, I think it's a good idea). Regarding the latter part, we only treat Python infra code a bit differently (but that'll hopefully go away soon).

zcorpan commented 5 years ago

resources/ is project infrastructure, common/ is useful helper utilities that aren't essential infrastructure.

This seems like a useful distinction.

Stuff that should probably move out of resources (probably not into common as they seem to be specific to one feature):

As for common, some things there should probably also be moved out or removed (like canvas-*), and then it would be nice to have a better directory structure:

zcorpan commented 5 years ago

When trying to implement the above reorg I changed my mind a bit and left some files as top-level.

I'm also not certain yet how/where to best write documentation for these.

https://github.com/web-platform-tests/wpt/pull/19514

annevk commented 5 years ago

Some feedback:

Documenting and expanding upon common is greatly appreciated though.

foolip commented 5 years ago

If a lot of renaming is proposed, then that may well affect Chromium's use of WPT, and I'd suggest going through the RFC process.

zcorpan commented 5 years ago

Thanks for the feedback. The restructuring turned out to be much more involved than I first thought, and considering the cost+risk/benefit again I think we shouldn't follow through with it. I will create new PRs for documentation and moving things out of common that don't belong there.

Top-level resources being special is weird, but I think it's best to explain this with documentation.