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

Avoid harness error in applyContraints/getConstraints test #18931

Closed
wants to merge 4 commits into from

Conversation

foolip
Copy link
Member

@foolip foolip commented Sep 9, 2019

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.

assert_object_equals(constraintsOut, constraintsIn, "constraints");
t.done();
});
await videoTrack.applyConstraints(constraintsIn);
Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

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?

@foolip
Copy link
Member Author

foolip commented Sep 9, 2019

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);
Copy link
Contributor

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?

@foolip
Copy link
Member Author

foolip commented Sep 12, 2019

Looks like Taskcluster was failing due to #18930 (fixed in #18962) so results for Chrome and Firefox weren't produced. I'll rebase this branch to retrigger.

@foolip
Copy link
Member Author

foolip commented Sep 20, 2019

@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 getContraints().

@gsnedders gsnedders closed this Jan 24, 2020
@gsnedders gsnedders deleted the foolip/fix-getConstraints branch January 24, 2020 18:06
@gsnedders gsnedders restored the foolip/fix-getConstraints branch January 24, 2020 18:51
@Hexcles Hexcles reopened this Jan 24, 2020
@foolip foolip closed this Oct 11, 2022
@foolip foolip deleted the foolip/fix-getConstraints branch October 11, 2022 09:36
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

5 participants