Document test updates associated with amendments #2910
Conversation
|
There are WebIDL validation steps failing, can you take a look @dontcallmedom ? |
|
I did - I think they're bogus (since this PR doesn't touch the document at all), but in any case, I can spend more time on them once there is approval at least with the principles and the data :) |
|
Ack, @jan-ivar to take a closer look, but free free to merge if he is happy about doing so |
|
PTAL @jan-ivar :) |
Fixes: #2916. The containing sentence talks about the derivation of the attributes of RTCIceCandidate, and so it should link to the attibutes of RTCIceCandidate and not to the members of RTCIceCandidateInit.
Ensure generation of valid markup
| } | ||
| const validTestUpdates = ["already-tested", "not-testable"]; | ||
| if (typeof prAmendment.testUpdates === "string" && !validTestUpdates.includes(prAmendment.testUpdates)) { | ||
| core.setFailed(`Pull request ${context.issue.number} declares an invalid test status in its amendment testUpdates field`); |
There was a problem hiding this comment.
Does it mandate to have the test at the time we add the ammendment?
There was a problem hiding this comment.
for spec changes that imply a change in the test suite, it requires a link to a WPT pull request at the moment, indeed (but that's distinct from it being implemented anywhere); this could be relaxed, but personally, I think there is real value in requiring an update to the test suite before merging a test-impacting amendment.
There was a problem hiding this comment.
I understand this but, without an implementation, those tests will be low quality probably, might break and so on.
At least we should flag these tests or make easy to manage/understand they are still experimental.
|
If the tool requires to put a test status in the amendment, we might need a 'to-be-added-shortly' state, as implementation might not be yet available. |
Co-authored-by: youennf <youennf@users.noreply.github.com>
| @@ -169,6 +234,12 @@ | |||
| { | |||
| "description": "Add RTCIceCandidate.relayProtocol", | |||
| "pr": 2763, | |||
| "tests": [ | |||
| "idlharness.https.window.js" | |||
There was a problem hiding this comment.
Just a note: idlharness.https.window.js tests that relayProtocol exists (1 implementation), but no WPT tests exist to verify it returns anything other than null, which might be difficult to test without a TURN server.
| "description": "Add RTCIceCandidate.url", | ||
| "pr": 2773, | ||
| "tests": [ | ||
| "idlharness.https.window.js" |
There was a problem hiding this comment.
Just a note: idlharness.https.window.js tests that url exists (1 implementation), but no WPT test exists to verify it returns anything other than null, which might be difficult to test without a TURN server.
Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>
This is a first pass at associating amendments to the relevant test cases and updates made to WPT as part of the amendment documenting process.
I have not documented associated test updates for the following PRs: #2758 #2759 #2794 #2797 #2801 #2847 #2853 #2861 - most likely because no updates were brought, but it could be that I missed them too.
It would be useful if someone could review that the annotations made in
amendments.jsonare correct. Namely that:Merging this PR will also require that any new amendment documents its test status (either as "already-tested", "not-testable", or as an array of pointers to WPT pull requests).
Preview | Diff