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 step_timeout in PresentationConnection_onxxx #4319

Merged

Conversation

obstschale
Copy link
Contributor

This adds step_timeout to all 3 PresentationConnection_on* tests and relates to number 1 in #4040

@wpt-pr-bot
Copy link
Collaborator

Notifying @louaybassbouss, @tidoust, and @zqzhang. (Learn how reviewing works.)

@tidoust
Copy link
Contributor

tidoust commented Dec 13, 2016

Unless I missed something, this code will not make the test harness report a "timeout" status. There needs to be a call to force_timeout before the call to done, as in:
https://github.com/w3c/web-platform-tests/blob/master/presentation-api/controlling-ua/startNewPresentation_success-manual.html#L58

Also, could you update the timeout to 5 seconds instead of 2, as in other tests?

@obstschale
Copy link
Contributor Author

I did so. I thought 2 sec would be enough.

Can you explain why we need to call force_timeout before done?

@tidoust
Copy link
Contributor

tidoust commented Dec 13, 2016

I thought 2 sec would be enough.

Right. 2s might be enough, but 5s is the default timeout of the test harness so we should stick to it unless there's a good reason to depart. In an ideal world, we would not need to hardcode this number in tests but would rather have a method to tell the harness to just restart the timeout using whatever duration it uses by default. That method does not exist though.

Can you explain why we need to call force_timeout before done?

Well, step_timeout merely sets a timeout in the JS sense of that world, it does not tell the test harness to consider that the test times out after some duration.

In other words, If you just call done, all you're telling the test harness is "look, I'm done with this test". Since no expectation failed, the test harness will happily report a PASS outcome. The call to force_timeout tells the test harness "look, you should consider that this test timed out". The test harness will then report a TIMEOUT outcome.

@tidoust tidoust merged commit 2a36110 into web-platform-tests:master Dec 13, 2016
@obstschale obstschale deleted the PresentationConnection_on-tests branch December 13, 2016 17:25
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