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
Avoid harness error in applyContraints/getConstraints test #18931
Conversation
assert_object_equals(constraintsOut, constraintsIn, "constraints"); | ||
t.done(); | ||
}); | ||
await videoTrack.applyConstraints(constraintsIn); |
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.
Note that this changes the behavior of the test. This being rejected is no longer tolerated. I tried wrapping this in try/catch, but even if one suppresses the error the following assert still fails. Feedback appreciated.
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.
My recollection is that async_test
runs in parallel while promise_test
runs in sequence. The failure may be caused by the original test being buggy. Can you upload the error where you wrap the applyConstraints()
call in a try/catch to ignore any errors?
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.
That's right, promise_test
run in sequence. The way this was written it should have been fine to race all the subtests with async_test
, I switched to promise_test
just to make the test clearer.
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.
I've pushed a change with the try/catch to show what happens.
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.
I'm sorry but I'm having some trouble navigating the CI results. What is the error causing the failure? If it is an assert_object_equals()
what are the actual and expected values?
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.
After we were embarrassed at TPAC to realize how obscure the "Expand" button is and you learned of its existence, can you tell from the results at which step this failed and if you think this is the right failure mode?
https://wpt.fyi/results/mediacapture-image/MediaStreamTrack-getConstraints-fast.html?run_id=313930003&run_id=303090005&run_id=277420005 shows that this will cause all tests to fail in all browsers instead. Reviewers, input much appreciated on what this test should do. Context: consistent harness error usually indicate a problem with the test, and there aren't very many of them. It would be nice if there were none and we could add tooling to prevent them from being introduced. |
assert_object_equals(constraintsOut, constraintsIn, "constraints"); | ||
t.done(); | ||
}); | ||
await videoTrack.applyConstraints(constraintsIn); |
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.
My recollection is that async_test
runs in parallel while promise_test
runs in sequence. The failure may be caused by the original test being buggy. Can you upload the error where you wrap the applyConstraints()
call in a try/catch to ignore any errors?
Because the promise callbacks containing the asserts weren't wrapped in `t.step_func` a failed assert turned into a harness error: https://staging.wpt.fyi/results/mediacapture-image/MediaStreamTrack-getConstraints-fast.html?run_id=227500002&run_id=259130004&run_id=253190002&run_id=247230003 This masked that the tests were actually failing.
5dde303
to
abdd2a9
Compare
@reillyeon https://wpt.fyi/results/?pr=18931&label=pr_head&max-count=1 is the view to look at. The failure still isn't very obvious, but what's happening is that the "advanced" property is missing from the return value of |
Because the promise callbacks containing the asserts weren't wrapped
in
t.step_func
a failed assert turned into a harness error:https://staging.wpt.fyi/results/mediacapture-image/MediaStreamTrack-getConstraints-fast.html?run_id=227500002&run_id=259130004&run_id=253190002&run_id=247230003
This masked that the tests were actually failing.