-
Notifications
You must be signed in to change notification settings - Fork 180
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
Ask for tests for normative changes in CONTRIBUTING.md #653
Conversation
See https://github.com/foolip/testing-in-standards/blob/master/policy.md for context. This phrasing matches that used for many other specs' CONTRIBUTING.md files.
Some additional context. The reason I came here was that in https://foolip.github.io/day-to-day/ I could see that Web Authentication has fairly high activity in the spec compared to the tests: I'm not presupposing that you want to adopt this working mode, sending this PR is just to open the discussion. |
I think this is a great idea. |
@jcjones, great! Is there a meeting where this should be discussed, or what's the next step? |
@jcjones, are you going to discuss this at TPAC on Thursday? I won't be able to make it I'm afraid, but if there's any information I can provide up front let me know. |
Yes, I think we should discuss it along with moving to CR at TPAC on Thursday. @nadalin, can you add this to the agenda? |
@mikewest FYI, I won't be there, can you drive this? |
@mikewest tells me that this might involve mocking hardware. Here's some other work in the area:
|
I think the WebUSB model is closest to what we want to do. I'm just starting to put together plans for a software security key for Chrome just for this purpose. I definitely think we should discuss interfaces to expose before getting too far down that road. |
do we need to have the stuff discussed herein all figured out in order to attempt to attain CR? |
@equalsJeffH, no, all of this is independent of W3C process. At some point it should be demonstrated that there are interoperable implementations, and a solid test suite should help with that, but it's possible to get through the process with a very unsolid test suite too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see questions below...
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrt this sentence "If testing is not practical..." -- is seems to be in reference to a presumed normative-but-not-practically-testable webauthn PR, yes?
wrt the "Add the type:untestable
or type:missing-coverage
label..." sentence -- add the label to the presumed normative-but-not-practically-testable webauthn PR or to the newly-submitted web-platform-tests issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd expect that some aspects of webauthn would be hard to test and would make sense to file issues for rather than block the spec changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@foolip
Ok, so might you please be able to appropriately update your proposed language above? thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think of my comments as proposing changes, so I'm confused :)
Is it just to spell out where the labels go? How about "Add the type:untestable
or type:missing-coverage
label as appropriate to the new issue."?
this PR is awaiting an update to the proposed additions to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear that there's working group consensus to do this. In particular, most contributors to the spec do not know how to contribute to the tests. Imposing this requirement could prevent people from making needed improvements to the specification.
There are already a bunch of tests in https://github.com/web-platform-tests/wpt/commits/master/webauthn, and when changing the spec the risk is that the tests end up being wrong, contradicting the spec. In the worst case, implementers won't even notice, and will match the tests instead of the spec. The objective here is not testing as such, but reaching interoperability on spec changes faster. I don't know if there are special testing challenges for webauthn, but experience from other groups suggests that progress is faster even though there is more up-front work. I think a reasonable minimum bar is that when normative spec changes are made that aren't reflected in tests, then an issue is filed to keep track of that. Or some other mechanism that helps ensure that changes aren't just silently forgotten, which tends to happen when the spec changes arent't closely followed by all implementers and there's nothing else to serve as a reminder of the required implementation change. But, it's ultimately up to the people doing the work, I am merely suggesting a workflow that I think would work. |
this is a process work-mode suggestion and IMV does not need to be tied to the L2-WD-01 milestone... |
This change is over four years old now and seems to be lacking someone to drive it to completion so the WG concluded that it should be closed during the call of 2021-12-15. The Web Platform Tests are getting updated in line with spec changes so the process seems to be working ok here already. |
See https://github.com/foolip/testing-in-standards/blob/master/policy.md
for context. This phrasing matches that used for many other specs'
CONTRIBUTING.md files.