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

"Common" files are untested and poorly documented #17913

Open
jugglinmike opened this issue Jul 18, 2019 · 17 comments
Open

"Common" files are untested and poorly documented #17913

jugglinmike opened this issue Jul 18, 2019 · 17 comments

Comments

@jugglinmike
Copy link
Contributor

@jugglinmike jugglinmike commented Jul 18, 2019

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:

  • define a convention for documenting each file (see below)
  • document each file
    • *.js files documented using JSDoc comments #19565
    • *.py files documented using reStructuredText in docstrings, like wptserve
    • Other files documented in README.md #19548
  • extend the documentation website build process to consume that pattern and generate a page for the common code
  • extend the lint tool to require documentation
  • write tests for the code (likely stored in a new subdirectory of infrastructure/)
  • remove files that are no longer used
  • relocate files that are used sparingly

/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

This comment has been minimized.

Copy link
Member

@Hexcles Hexcles commented Jul 18, 2019

Related #11256

@Hexcles

This comment has been minimized.

Copy link
Member

@Hexcles Hexcles commented Jul 18, 2019

  • 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

This comment has been minimized.

Copy link
Contributor Author

@jugglinmike jugglinmike commented Jul 18, 2019

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

This comment has been minimized.

Copy link
Contributor

@foolip foolip commented Jul 18, 2019

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

This comment has been minimized.

Copy link
Contributor Author

@jugglinmike jugglinmike commented Jul 18, 2019

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

@Ms2ger

This comment has been minimized.

Copy link
Contributor

@Ms2ger Ms2ger commented Jul 29, 2019

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

This comment has been minimized.

Copy link
Contributor

@gsnedders gsnedders commented Jul 29, 2019

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

This comment has been minimized.

Copy link
Contributor Author

@jugglinmike jugglinmike commented Jul 29, 2019

Is that still the intent today?

@gsnedders

This comment has been minimized.

Copy link
Contributor

@gsnedders gsnedders commented Jul 29, 2019

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

This comment has been minimized.

Copy link
Contributor Author

@jugglinmike jugglinmike commented Jul 29, 2019

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.

@zcorpan zcorpan self-assigned this Oct 3, 2019
@jgraham

This comment has been minimized.

Copy link
Contributor

@jgraham jgraham commented Oct 3, 2019

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

This comment has been minimized.

Copy link
Member

@Hexcles Hexcles commented Oct 3, 2019

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

This comment has been minimized.

Copy link
Member

@zcorpan zcorpan commented Oct 3, 2019

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):

  • resources/check-layout-th.js
  • resources/SVGAnimationTestCase-testharness.js
  • resources/sriharness.js

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:

  • Pages (blank.html, dummy.xhtml, dummy.xml, text-plain.txt)
  • wptserve python handlers (*.py)
  • Utility scripts (*.js)
  • Data (entities.json)
zcorpan added a commit that referenced this issue Oct 4, 2019
Part of #17913

WIP: references to these files have not yet been updated.
@zcorpan

This comment has been minimized.

Copy link
Member

@zcorpan zcorpan commented Oct 4, 2019

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.

#19514

@annevk

This comment has been minimized.

Copy link
Member

@annevk annevk commented Oct 6, 2019

Some feedback:

  • If top-level resources/ is for infrastructure, it's weird that we have many non-top-level resources/ that are not.
  • I don't like making common/ less flat as it'll make it harder to remember the URLs then. No need for these to end up like doctypes before <!doctype html>.

Documenting and expanding upon common is greatly appreciated though.

@foolip

This comment has been minimized.

Copy link
Contributor

@foolip foolip commented Oct 6, 2019

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

This comment has been minimized.

Copy link
Member

@zcorpan zcorpan commented Oct 7, 2019

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.

zcorpan added a commit that referenced this issue Oct 7, 2019
Part of #17913.
jgraham added a commit that referenced this issue Oct 7, 2019
Part of #17913.
zcorpan added a commit that referenced this issue Oct 8, 2019
Part of #17913.
zcorpan added a commit that referenced this issue Oct 9, 2019
Also remove common/namespaces.js and fix affected files.

Part of #17913.
zcorpan added a commit that referenced this issue Oct 9, 2019
Part of #17913.
zcorpan added a commit that referenced this issue Oct 9, 2019
zcorpan added a commit that referenced this issue Oct 9, 2019
zcorpan added a commit that referenced this issue Oct 9, 2019
zcorpan added a commit that referenced this issue Oct 9, 2019
zcorpan added a commit that referenced this issue Oct 9, 2019
Part of #17913.
zcorpan added a commit that referenced this issue Oct 9, 2019
zcorpan added a commit that referenced this issue Oct 10, 2019
Part of #17913.
zcorpan added a commit that referenced this issue Oct 10, 2019
zcorpan added a commit that referenced this issue Oct 10, 2019
gsnedders added a commit that referenced this issue Oct 11, 2019
Also remove common/namespaces.js and fix affected files.

Part of #17913.
yoavweiss added a commit that referenced this issue Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.