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

Adopt "test as you commit" policy #1617

Closed
foolip opened this issue Oct 4, 2017 · 8 comments · Fixed by #1657
Closed

Adopt "test as you commit" policy #1617

foolip opened this issue Oct 4, 2017 · 8 comments · Fixed by #1657

Comments

@foolip
Copy link
Member

foolip commented Oct 4, 2017

An increasing number of groups are beginning to make web-platform-tests part of their day-to-day work, and more are considering it, for example in w3c/das-charter#48.

In a Improving interop with web-platform-tests talk (slides 16-17 and 23) I gave here at the https://webengineshackfest.org/ I outlined the strategy, "tipping point" idea and the remaining work.

WebRTC is at the same time challenging and in an unusually good situation testing-wise. The challenge is that http://wpt.fyi/webrtc is very red because of a lack of automation. Chromium's ecosystem infra team is investing in fixing this in Q4 (search for "WebRTC").

What makes things better for WebRTC than most places is that testing activity is already very high (see visualization) and a lot of work was done recently by @soareschen and @rwaldron to improve the test suite. See https://rwaldron.github.io/webrtc-pc/ for an an annotated version of the spec.

It could be something similar to the SVG policy, meaning:

  • Those who review spec changes ask for a corresponding PR in web-platform-tests.
  • If testing is currently impossible or impractical, issues with the type:untestable or type:missing-coverage labels are filed.
  • There are no hard rules, just a common understand of what's expected, analogous to code review practicies.

WDYT?

@stefhak
Copy link
Contributor

stefhak commented Oct 10, 2017

My personal view is that this sounds like a good WoW, but before we can adopt it the current test suite must decently covering (though it is in pretty good shape in this case as you point out).

Our plan is to discuss the test suite at TPAC (proposed agenda: https://lists.w3.org/Archives/Public/public-webrtc/2017Oct/0012.html), perhaps we can talk about this subject in conjunction with that?

I also think we should not adopt a new WoW before we've transited to CR (something I hope we will manage real soon).

@foolip
Copy link
Member Author

foolip commented Oct 10, 2017

but before we can adopt it the current test suite must decently covering (though it is in pretty good shape in this case as you point out)

Do you mean that coverage needs to be better than indicated by https://rwaldron.github.io/webrtc-pc/, or just that OK coverage is a prerequisite which is already met? Of the ~80 specs that have adopted a similar working mode so far, there were none where the detailed coverage was even known, and probably only a handful (maybe DOM and Service Worker) where the coverage was this good.

In practice, when making a change one goes to see if there's already coverage, and if there isn't and it's too much work to build up the required scaffolding, one just files an issue about the fact and move along without tests.

@stefhak
Copy link
Contributor

stefhak commented Oct 10, 2017

In practice, when making a change one goes to see if there's already coverage, and if there isn't and it's too much work to build up the required scaffolding, one just files an issue about the fact and move along without tests.

That sounds like a good way to handle my fear (i.e. that people would not contribute PRs/patches since they would have to develop the tests for the already existing features their patch depend on).

@stefhak
Copy link
Contributor

stefhak commented Oct 12, 2017

@annevk
Copy link
Member

annevk commented Oct 12, 2017

Note also that it's fine to leave PRs open for a while until you can find sometime to tie up the lose ends. It doesn't have to be the same person writing the PR and the tests. But please if you find things not tested try to get someone on that, that's a major liability for the stability of the specification.

@foolip
Copy link
Member Author

foolip commented Nov 6, 2017

We have just finished discussing this at TPAC, and there is consensus to try this out. There is a caveat that if it doesn't work out, then the group will re-evaluate. I'll go ahead and prepare a PR.

@youennf
Copy link
Contributor

youennf commented Nov 6, 2017

Some other groups try to file bugs on browser bug trackers at the same time.
This is an efficient way to push for implementation of such changes.

@foolip
Copy link
Member Author

foolip commented Nov 6, 2017

That's right, that's something I would also encourage, especially if there's extra context you can provide that will be hard to get from the failing test alone, like "this break real-world sites".

foolip added a commit to foolip/webrtc-pc that referenced this issue Nov 6, 2017
At TPAC it was resolved to try this policy for webrtc-pc.

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

Successfully merging a pull request may close this issue.

4 participants