Skip to content

Conversation

@jugglinmike
Copy link
Contributor

Prior to this commit, the test
MediaStreamTrack-getConstraints-fast.html triggered asynchronous
operations in parallel. This represented a race condition because for
each subtest, two operations could trigger completion.

Additionally, the test made assertions asynchronously. Because these
assertions were not "wrapped" in the harness's "step" functions,
assertion failures would not be reported as test failures. Failures
would instead be reported inaccurately as harness errors (though if the
browser under test does not implement the unhandledrejection event,
the failed assertion would not be reported at all).

Schedule each pair of asynchronous operations to happen in series, and
ensure that all failed assertions are accurately reported as test
failures.

Prior to this commit, the test
`MediaStreamTrack-getConstraints-fast.html` triggered asynchronous
operations in parallel. This represented a race condition because for
each subtest, two operations could trigger completion.

Additionally, the test made assertions asynchronously. Because these
assertions were not "wrapped" in the harness's "step" functions,
assertion failures would not be reported as test failures. Failures
would instead be reported inaccurately as harness errors (though if the
browser under test does not implement the `unhandledrejection` event,
the failed assertion would not be reported at all).

Schedule each pair of asynchronous operations to happen in series, and
ensure that all failed assertions are accurately reported as test
failures.
@reillyeon
Copy link
Contributor

This change LGTM but I am wondering if there are additional race conditions.

Since the generated async tests are reusing the same <canvas> element should they be converted to promise tests so that they run sequentially rather than attempting to apply a bunch of different constraints in parallel? That seems to depend on whether or not captureStream() is guaranteed to return fully independent MediaStream instances.

@jugglinmike
Copy link
Contributor Author

Yeah, that is a little concerning. I'm a bit reluctant to make changes like that because I don't have a working reference implementation. This change is minimal, but it demonstrably improves the results (by reporting 16 failure and 16 passes instead of 32 passes and a harness error).

I'm happy to follow up with a patch and let you folks decide, though. As for this patch, it's currently blocked on some temporary problems in Mozilla's infrastructure. I'll run another trial tomorrow and plan to merge once it passes.

Thanks for the review!

@jugglinmike jugglinmike reopened this Nov 14, 2019
@jugglinmike jugglinmike merged commit a3a6741 into web-platform-tests:master Nov 14, 2019
@jugglinmike jugglinmike deleted the mediacapture-image-unhandled-rejection branch November 14, 2019 20:17
@jugglinmike
Copy link
Contributor Author

Following up with the requested de-parallelization in gh-20262

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.

3 participants