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

Add RTCDataChannel-binaryType.html from PR13499 #16043

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Mar 24, 2019

No description provided.

@youennf
Copy link
Contributor Author

youennf commented Mar 24, 2019

@lgrahl, made some changes to the original test. PTAL.

Copy link

@lgrahl lgrahl left a comment

Choose a reason for hiding this comment

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

LGTM apart from a nit.

webrtc/RTCDataChannel-binaryType.window.js Outdated Show resolved Hide resolved
webrtc/RTCDataChannel-binaryType.window.js Outdated Show resolved Hide resolved
@youennf
Copy link
Contributor Author

youennf commented Mar 24, 2019

Sure, I can remove them, they kind of make sense to me though.
The test is about setting a value.

Before setting, check the value.
After setting, check the value.
After failing to set the value, check the value.

@lgrahl
Copy link

lgrahl commented Mar 24, 2019

Works for me then.

@alvestrand
Copy link
Contributor

@youennf if this is ready and still useful, can you hit merge?

redundant with other tests
@dontcallmedom
Copy link
Contributor

@foolip can you help me understand what's making the CI stuck on this PR? (if not you, who should I ping?)

@stephenmcgruer
Copy link
Contributor

@foolip can you help me understand what's making the CI stuck on this PR? (if not you, who should I ping?)

You can always ping @web-platform-tests/wpt-core-team for a more general set of people that can help :)

The problem here is that the PR is so old that there have been several infra shifts on the Taskcluster side that make old PRs incompatible. That said, if reviewers are happy with it and the test seems solid, I'm happy to override the CQ and admin-merge it - please let me know.

@dontcallmedom
Copy link
Contributor

@stephenmcgruer then please go ahead and admin-merge - thanks!

@frivoal
Copy link
Contributor

frivoal commented Oct 2, 2020

Force merging PRs with lint errors isn't nice, as it blocks every other PR. Now fixed by #25944

@sideshowbarker
Copy link
Member

Now fixed by #25944

Thanks for catching that and fixing it

Force merging PRs with lint errors isn't nice, as it blocks every other PR.

Nobody realized it had lint errors. Those went unnoticed among the other CI errors it was triggering due its age (see previous comments here) and was force-merged because those CI errors were non-blockers for this case. Nobody would have intentionally merged it knowing it had lint errors as well.

@stephenmcgruer
Copy link
Contributor

Force merging PRs with lint errors isn't nice, as it blocks every other PR. Now fixed by #25944

Echoing Mike; thanks for catching this and taking care of it! My apologies for not considering that as a possibility, we should considering taking a(n unofficial?) policy of cherry-picking changes like this one to new PRs rather than force-merging the original PR. (Or we could see if we can fix Taskcluster to not rely on the base commit for some of its mechanisms, cc @jgraham for that).

It's a pity that even when GitHub claim its the default, most PRs from forks that I come across I cannot modify in any way and instead have to rely on the original author for minor fixups or rebases.

@jgraham
Copy link
Contributor

jgraham commented Oct 5, 2020

FWIW I think the best option here would have been to simply rebase the branch. If we didn't have access to the underlying branch it might have indeed been good to create a new PR.

ziransun pushed a commit to ziransun/wpt that referenced this pull request Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.