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

Make PresentationConnection.onclose pass #4222

Conversation

obstschale
Copy link
Contributor

This is a PR for #4040

To make this test pass a promise_test is used.

I also changed 2 things in presentation-api/controlling-ua/common.js.

  1. I fixed the castUrl. The castClientId was missing. That caused a ErrorNotFound rejection.
 var castClientId = String(new Date().getTime()) + String(Math.floor(Math.random() * 1e5)); 
  var castUrl = 'https://google.com/cast#__castAppId__=' + castAppId + '/__castClientId__=' + castClientId; 
  1. I switched the order of urls in the array. So the google cast url comes first. Otherwise Google rejects the url and does not take the second url.

To make this test pass a `promise_test` is used.
@wpt-pr-bot
Copy link
Collaborator

Reviewers for this pull request are: @louaybassbouss, @tidoust, and @zqzhang.

@tidoust
Copy link
Contributor

tidoust commented Nov 21, 2016

Hi @obstschale, see step_timeout comment in #4040 (comment)

About the changes in common.js, I do not know which configuration you are using to run the tests, but the changes you propose do not fix anything for me. As far as I can tell:

  1. Changing the order of the URLs in the array does not change anything. The problem is that implementations do not support the PresentationRequest constructor that takes an array parameter.
  2. Another problem is that Cast devices do not implement the receiving side of the Presentation API. Most tests fail as a result because the user agent cannot establish a communication channel with the receiving app.
  3. Also (but that does not have any effect in practice given the fact that the receiving side is not yet implemented anywhere), there's a missing } in the current receiver app support/presentation.html, which means that the page crashes with a JS error when it loads.
  4. castClientId is no longer needed, I think.

Points 1. and 2. should hopefully be temporary: support should improve over time.

Point 1 can of course be solved in the meantime with code such as window.presentationUrls = castUrl but that is not what we want for the test suite longer term. It should be possible to use the Presentation API with a "regular" non-Cast-bound HTTP URL and that is the default scenario that the test suite should aim at testing. The current design and order of URLs in the array is very much on purpose.

For point 2, if there are no plans to support the receiving side of the Presentation API on Cast devices for the foreseeable future, we may need to create a specific Cast receiver app (that uses the Cast SDK) in the end. This would allow to run additional tests on the controlling side.

Both points mean that most tests currently fail in practice, but I don't think that is a bug with the test suite. This makes it hard to author tests for sure. I'll give it a try with a true Cast receiving app to see if we can ease our lives while implementations catch up.

I would suggest to address this separately from the updates on the on_xxx tests.

@obstschale
Copy link
Contributor Author

That is all still very confusing to me 🤔

Why did I change common.js?

I tested var request = new PresentationRequest(presentationUrls); with different URLs and the array from common.js. My tests always fail when I use the predefined URLs or array with: "NotFoundError: No screens found.". However if I change the google cast URL so it holds a castClientId it seems to work. And if I give an array where this changed google cast URLs is on index 0 it works as well.

Using support/presentation.html does not work for me.

1. Browser Implementation

So if browsers don't support an array as constructor param, why do we use them in common.js? I mean, how can I make sure my test is correct if I write one which I cannot verify?
I do understand, that browser should implement the specs and the test should cover the spec not the current browser implementation. But the question remains: How can I make sure the test is correct?

Both points mean that most tests currently fail in practice, but I don't think that is a bug with the test suite. This makes it hard to author tests for sure. I'll give it a try with a true Cast receiving app to see if we can ease our lives while implementations catch up.

Ok, you test it with a true Cast app. Can I also test it with a true app?

@tidoust
Copy link
Contributor

tidoust commented Nov 28, 2016

Could you clarify what test configuration you're using?

I have a Chromecast (running Google Cast v1.21.74816) for the receiving side. Google Chrome for Android (v54.0.2840.85) seems happy with the array constructor, picks up the Cast URL, regardless of its position in the array and shows my Chromecast in the list of devices available. Google Chrome for desktop (v54.0.2840.99m) and Chrome Canary (v57.0.2931.0) on Windows do not seem to support the array constructor at all. I get an empty list of devices (provided I force the source to be the controlling page as Chrome proposes to mirror the tab as a fallback) and a NotFoundError if I dismiss the dialog box. Changing the order of the URLs does not change anything. The only setting that makes my Chromecast appear is if presentationUrls is equal to the Cast URL.

In short, I get mixed results depending on what I use, but the order of URLs in the array and castClientId do not change anything for me. The castClientId is perhaps still needed, e.g. to bypass the cache. It just does not seem to be.

Regardless of these results, we want to create a test suite that can run across implementations of the Presentation API. Such tests should not contain proprietary code. Cast URLs are proprietary: the Presentation API does not define how a user agent is to handle a Cast URL and map it onto the address of the page to present. In theory, the test suite should not contain any reference to a Cast URL. It does in practice because that's the only available receiver right now, plus it allows to test the 2-UA mode. In an ideal world, we'd be able to get rid of Cast URLs soon.

Another problem we have to face today is the lack of implementations of the receiving side. support/presentation.html cannot load in the meantime, and some tests fail as a result.

how can I make sure my test is correct if I write one which I cannot verify?

Well, you cannot! This is what makes writing tests so hard. The only things you can do are:

  1. make sure your test code follows the spec algorithms
  2. keep the test code as simple and readable as possible
  3. get your code reviewed

Of course, you can change common.js in your local fork of the Web Platform Tests repository, but note that won't solve the bigger problem that there are no available implementations of the receiving side for the time being.

Can I also test it with a true app?

By "true Cast app", I mean one that follows the initialization steps in the Custom Receiver Application Google Cast doc.
What do you call a "true app"? support/presentation.html?

@obstschale obstschale closed this Dec 12, 2016
@obstschale obstschale deleted the PresentationConnection-test-fix branch December 12, 2016 20:47
@obstschale
Copy link
Contributor Author

I had to close this PR because I merged some stuff from #4256 and that would make this PR really messy I guess.

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.

None yet

3 participants