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

[all] Consider policy to ask for web-platform-tests #1755

Closed
zcorpan opened this issue Aug 23, 2017 · 7 comments

Comments

Projects
None yet
5 participants
@zcorpan
Copy link
Member

commented Aug 23, 2017

See w3c/fxtf-drafts#215 for this for fxtf.

I would like to adopt this policy for cssom and cssom-view. Having tests and browser bugs for each normative spec change has turned out to be a really good way to reduce the time it takes for that change to be interoperably implemented, which is one of the main goals of standards.

We have lots of specs here. I don't know if this policy makes sense for all CSS specs. But I think it does for most of them. I would be happy to do this for only cssom and cssom-view for now, but I'd like the CSS WG to consider this for every spec, so we can decide what's best and apply that.

Case study: Chromium recently sent an Intent to Implement and Ship for the 'q' unit, and found that there was only a test for it for <img sizes>, there was no CSS-specific test. With this policy, there would be some tests written and ideally also browser bugs filed asking to implement at the same time as it was added to the specification.

@zcorpan zcorpan added the Agenda+ label Aug 23, 2017

@frivoal

This comment has been minimized.

Copy link
Collaborator

commented Aug 23, 2017

At the last F2F, we've agreed to require tests (or a good explanation of why you cannot) for all changes to specs ≥ CR. Before that, tests are recommended but not mandatory, to avoid test churn and work overload on know-to-be unstable specs or sections.

The wording you're proposing in that PR seems compatible with that approach.

Minutes here: https://logs.csswg.org/irc.w3.org/css/2017-08-03/#e843981 (The "Automating Test Coverage" topic title is inacurate: two topics got blended in one)

Do you think we need anything else?

@foolip

This comment has been minimized.

Copy link
Member

commented Aug 24, 2017

I wasn't at the F2F, but I read the minutes with great interest afterwards. Did the resolution ("Changes List for a CR links to updated or additional test cases for each change") get turned into a written policy somewhere? This is the biggest missing piece, in my view.

Ideally, I think that the policy shouldn't depend on the formal status, since exceptions for good (other) reasons should be OK before and after CR. The policy suggested by @zcorpan here is (deliberately) worded rather weakly, and allows for practically any organization of testing work as long as missing test coverage is tracked. So, it would be reasonable for a spec that is very unstable to simple have an "there are no tests" issue, then replaced by a more concrete TODO list as the spec matures, and eventually something approximating full coverage.

https://drafts.csswg.org/cssom/ and https://drafts.csswg.org/cssom-view/ are both editor's drafts, but are both still clearly mature enough (similar to HTML/DOM) to have tests or issues for every normative change.

@fantasai

This comment has been minimized.

Copy link
Collaborator

commented Aug 26, 2017

Recording into a WG resolution makes it official written policy. :) The policy is tied to CR since CR specs (that are published by the CSSWG) are stable enough that they don't churn the way WDs do. Before CR it's at the discretion of whoever is writing the tests: of course the WG will not reject tests prior to CR. As for exceptions, they are always by WG resolution anyway, and substantive updates to CR specs require WG resolutions testcase or not.

Note that CSSOM and CSSOM-View in particular are largely about documenting existing features rather than designing new ones, so its particularly useful to have tests for them even though they are still WD--if anything, the tests help with acquiring and demonstrating the spec stability needed for us to transition them to CR. This is less the case for specs like CSS Grid Layout (when it was a WD) where there wasn't much in the way of existing implementations, interop with such implementations wasn't a goal, and the spec was going through (expected) significant churn as the model was being worked out: in such cases, writing a comprehensive test suite from day 1 would be a huge waste of effort that could have better been spent elsewhere (e.g. on CSSOM ;)

@foolip

This comment has been minimized.

Copy link
Member

commented Aug 28, 2017

Recording into a WG resolution makes it official written policy. :)

Right, that's "RESOLVED: Changes List for a CR links to updated or additional test cases for each change"

What I think would be helpful is if that's documented in CONTRIBUTING.md, spelling out some of the details about how to manage two pull requests and labels, "Needs Testcase" on this side and "status:needs-spec-decision" on the wpt side, among others.

Finally, it'd be neat to list the pre-CR specs which use the same working mode as well, cssom and cssom-view to begin with.

Does all of that make sense?

@zcorpan

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2017

A few thoughts...

  • I agree it would be good to document the policy in the repository, and not just have a WG resolution, to make it easier to find and to bring more confidence to the reader (a WG resolution might have been overridden later by a new WG resolution, for example).
  • As for timing when it makes most sense to start writing tests for each change, I suggest it is when a feature is shipping in one or more browsers, irrespective of spec status. Experience suggests there is some lag between a feature shipping and it reaching CR, and tests are necessary i the meantime.

In particular, I think writing tests should be culturally part of day-to-day work, as an integral part of fixing spec issues. Applying it to post-CR still seems like testing can be an after-thought, after it has been specified, implemented, shipped, and web content exist that rely on implementation differences that tests were supposed to prevent.

@foolip

This comment has been minimized.

Copy link
Member

commented Aug 28, 2017

@fantasai, on the topic of churn and writing tests from day 1, I agree that going too far would result in wasted effort. To transition from an untested and fast-changing spec to one that is and stays well tested, I suspect that one could ease in the testing discipline, much like in a software project.

I'm not advocating for formalizing that transition, I think the editors have to make that kind of call. As long as there's something written down that a spec like CSSOM could point to, that'd allow for experimentation.

@css-meeting-bot

This comment has been minimized.

Copy link
Member

commented Aug 30, 2017

The Working Group just discussed Consider policy to ask for web-platform-tests.

The full IRC log of that discussion <dael> Topic: Consider policy to ask for web-platform-tests
<dael> github: https://github.com//issues/1755
<dael> astearns: zcorpan_ would like to require tests for cssom and cssom view changes even though those specs aren't in cr. I think that's consistant with what we resolved in paris. Requiing this for tests not yet in cr also makes sense and it's up tot he spec author to make that determination.
<dael> astearns: zcorpan_ is that accurate?
<zcorpan_> Yep.
<zcorpan_> So how this works in whatwg and other places is that a spec PR is blocked on having tests, and the two PRs are merged at the same time
<dael> astearns: I am in favor of having spec authors require tests for changes they make at their discression in addition to having rossen and I hold people to that standad for CR+ specs
<dael> Florian: I'm a little confused. I thought zcorpan_ wanted to check in a file that explained that. It's nto the author requiring himself to do that, but for 3rd party contributors.
<dael> Florian: It's things added by everyone to that spec. poss merged by editor
<dael> astearns: think zcorpan_ shoudl add somethign saying ti's required for CR specs and also for other specs at some level of stability.
<dael> Florian: I think his wording was a should and it was reused from other places. zcorpan_ are you okay with more specific wording or do you want to keep the generic one?
<zcorpan_> I would be happy either way, whatever works best for the group
<fantasai> gsnedders, if we're looking into reorganizing the tests within css/, we should also look into moving those <200 non-cssom tests into css/; otherwise more people will try to copy that pattern and we'll end up with duplicate locations
<dael> astearns: I think I would prefer havign the more specific wording so that I have something to point to when I start being hard about this requirement.
<dael> astearns: Any objections to having more specific wording in test repo documentation?
<Rossen> +1
<Florian> astearns: +1
<zcorpan_> sounds good
<dael> astearns: I'll ask zcorpan_ to check those changes in.

@zcorpan zcorpan removed the Agenda+ label Aug 30, 2017

zcorpan added a commit that referenced this issue Aug 30, 2017

@zcorpan zcorpan closed this in #1767 Sep 4, 2017

zcorpan added a commit that referenced this issue Sep 4, 2017

Ask for web-platform-tests in CONTRIBUTING.md
This uses similar wording as in
w3c/fxtf-drafts#215

For the CSSWG, this policy is a must for CR+ and cssom/cssom-view.

Per WG discussion
#1755 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.