- Notifications
You must be signed in to change notification settings - Fork115
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does it mandate to have the test at the time we add the ammendment?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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>
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.
LGTM. Thanks! ❤️
| @@ -169,6 +234,12 @@ | |||
| { | |||
| "description": "Add RTCIceCandidate.relayProtocol", | |||
| "pr": 2763, | |||
| "tests": [ | |||
| "idlharness.https.window.js" | |||
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.
Just a note: idlharness.https.window.js tests thatrelayProtocol exists (1 implementation), but no WPT tests exist to verify it returns anything other thannull, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just a note: idlharness.https.window.js tests thaturl exists (1 implementation), but no WPT test exists to verify it returns anything other thannull, 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