-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Adding tests for remote playback API #35827
Adding tests for remote playback API #35827
Conversation
Just a drive-by comment that |
cc @mfoltzgoogle, as the bot has assigned the PR to Mounir despite w3c/remote-playback@4e2dc79 (It's probably a good idea to update |
Note I dropped Mounir as reviewer and assignee. I cannot add @muyao-xu or yourself as reviewer either though. |
Can I be added as a reviewer now? |
Sorry for the delay. It looks like no one was able to pick this up after TPAC last year. I will take a look. |
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.
Hi @FritzHeiden, overall these tests look really good! In some cases I made a comment on one test that applies to multiple tests, as the test structure is similar.
</div> | ||
<div id="pick-device" style="display: none"> | ||
<p> | ||
Click the button below to prompt for a remote playback device and select |
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.
This instruction needs updating.
<script src="/resources/testharness.js"></script> | ||
<script src="/resources/testharnessreport.js"></script> | ||
<script src="/common/media.js"></script> | ||
<style> |
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.
This is a super minor thing, but consider putting this in an external stylesheet so it can be used by all the tests.
remote-playback/state-attribute-changes-when-selecting-device-manual.html
Show resolved
Hide resolved
} | ||
|
||
v.remote.watchAvailability(t.step_func_done(callback)).then( | ||
t.step_func(() => {}), t.unreached_func()); |
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 legal for the browser to reject watchAvailability at any time. You can assert that the rejection is NotSupportedError
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.
The code looks very good. There are two tests I think could be merged and I can take a look again if you agree with that change.
I made some copy editing suggestions on the test descriptions to make them a little more precise (in my opinion). If you want to defer on these, they won't block approving the PR.
remote-playback/state-attribute-changes-when-selecting-device-manual.html
Show resolved
Hide resolved
remote-playback/disable-remote-playback-cancel-watch-availability-throws.html
Show resolved
Hide resolved
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.
None of my comments should prevent landing. I can put up a PR to address the remaining style/comment fixes separately. I'll have to re-read the code to see about combining the two test cases.
* Implemented some tests for remote-playback api * Implemented all API tests * Implemented all tests * Added test results * Made changes according to PR feedback
This PR introduces additional tests for the remote playback API