Skip to content

Ask for web-platform-tests in CONTRIBUTING.md #1767

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.

Sign up for GitHub

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

Merged
zcorpan merged 4 commits into from
Sep 4, 2017
Merged

Ask for web-platform-tests in CONTRIBUTING.md #1767

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ba13384
Ask for web-platform-tests in CONTRIBUTING.md
zcorpan Aug 30, 2017
a0953c2
Change policy to must for CR+ and cssom/cssom-view
zcorpan Aug 30, 2017
e86f5eb
Reword per astearns review
zcorpan Aug 30, 2017
4f127d6
Update per review comments
zcorpan Sep 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Change policy to must for CR+ and cssom/cssom-view 
Per WG discussion
#1755 (comment)
  • Loading branch information
@zcorpan
zcorpan committed Aug 30, 2017
commit a0953c2601e7547db0088b2debae27ba598bf60b
19 changes: 13 additions & 6 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,16 @@ feature, you can remove yourself with the above syntax.

# Tests

For normative changes, a corresponding
[web-platform-tests](https://github.com/w3c/web-platform-tests) PR is highly appreciated. Typically,
both PRs will be merged at the same time. Note that a test change that contradicts the spec should
not be merged before the corresponding spec change. If testing is not practical, please explain why
and if appropriate [file an issue](https://github.com/w3c/web-platform-tests/issues/new) to follow
up later. Add the `type:untestable` or `type:missing-coverage` label as appropriate.
For normative changes for any specification in
[CR or later](https://www.w3.org/Style/CSS/current-work) as well as the pre-CR specifications listed
below, a corresponding [web-platform-tests](https://github.com/w3c/web-platform-tests) PR must be
Copy link
Copy Markdown
Member

@foolip foolip Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been asking for a policy like this, so obvious this makes me happy, but I think it's important that there's some leeway here. The "testing is not practical" case will probably come up at least occasionally at CR or later, and if everyone agrees, then this "must" ought not get in the way of progress. I think that if this is downgraded to a "should", then editor's can use their best judgement about when to file issues instead of requiring tests.

Copy link
Copy Markdown
Member

@astearns astearns Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I'm reading it, the 'must' is to have a wpt PR or an explanation/issue for followup. It's still at the editor's discretion, but they must do one or the other.

Copy link
Copy Markdown
Member

@foolip foolip Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I'm reading this too much like a format spec, but I'm not able to parse it that way, "a corresponding PR must be provided" sounds quite final. Of course, it's only with RFC 2119 goggles that changing it to "should" helps. Interested to hear how others read this, and don't mind it if other have a more lenient interpretation.

Copy link
Copy Markdown
Collaborator

@fantasai fantasai Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if the intent is the way @astearns is reading it, then it needs to be reworded to be more clear about the either-or.

provided; for other specifications it is highly appreciated. Typically, both PRs will be merged at
Copy link
Copy Markdown
Collaborator

@fantasai fantasai Aug 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“for other specifications it is highly appreciated” I would probably downgrade this a bit to maybe s/highly/often/. I would not have appreciated people submitting tests to CSS Grid in the earlier exploratory stages of the spec: that just means more work for for us to go through the for-sure outdated and possibly entirely useless tests later on--and discourages submissions without tests even in cases where they wouldn't actually be helpful.

(Although, in general, I prefer people filing issues to PRs for normative spec changes anyway.)

the same time. Note that a test change that contradicts the spec should not be merged before the
corresponding spec change. If testing is not practical, please explain why and if appropriate
[file an issue](https://github.com/w3c/web-platform-tests/issues/new) to follow up later. Add the
`type:untestable` or `type:missing-coverage` label as appropriate.

The pre-CR specifications are currently:
Copy link
Copy Markdown
Member

@astearns astearns Aug 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "The pre-CR specifications with this testing requirement are currently:"

Copy link
Copy Markdown
Contributor Author

@zcorpan zcorpan Aug 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So changed.


* cssom
* cssom-view
Copy link
Copy Markdown
Collaborator

@fantasai fantasai Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

css-overflow-3 and css-position-3 will probably fall under this list as well, although I haven't looked at them lately, so fine to leave out for now.