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

canceling-an-animation.html has a harness error in all browsers #18932

Closed
foolip opened this issue Sep 9, 2019 · 16 comments

Comments

@foolip
Copy link
Contributor

@foolip foolip commented Sep 9, 2019

https://staging.wpt.fyi/results/web-animations/timing-model/animations/canceling-an-animation.html?run_id=227500002&run_id=259130004&run_id=253190002&run_id=247230003

Chrome and Edge have a harness error saying "The user aborted a request" and Firefox and Safari say "The operation was aborted".

@birtles @flackr @graouts @stephenmcgruer any idea what's going on here?

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.

@graouts

This comment has been minimized.

Copy link
Contributor

@graouts graouts commented Sep 9, 2019

This is due to this test:

test(t => {
  const animation = createDiv(t).animate(null);
  const promise = animation.ready;
  animation.cancel();
  assert_not_equals(animation.ready, promise);
}, 'The ready promise should be replaced when the animation is canceled');

The ready promise is rejected when calling cancel() and there is no code handling that promise's rejection. I suppose we could add:

promise_rejects(t, 'AbortError', promise, 'Cancel should abort ready promise');
@foolip

This comment has been minimized.

Copy link
Contributor Author

@foolip foolip commented Sep 9, 2019

Aha, thanks @graouts!

If there are other tests that check that animation.cancel() rejects then just animation.cancel().catch(() => {}) would probably be fine here. But are there?

@foolip

This comment has been minimized.

Copy link
Contributor Author

@foolip foolip commented Sep 9, 2019

Oh, sorry, it's not the return value of animation.cancel() that is rejected, it's the animation.ready promise.

@graouts

This comment has been minimized.

Copy link
Contributor

@graouts graouts commented Sep 11, 2019

@birtles does it sound right to you to make this change?

@stephenmcgruer

This comment has been minimized.

Copy link
Contributor

@stephenmcgruer stephenmcgruer commented Sep 11, 2019

It sounds right to me, if that helps :). Canceling an animation runs the reset an animation's pending tasks steps, which includes rejecting the ready promise.

graouts added a commit that referenced this issue Sep 11, 2019
…does not cause a harness error. This addresses issue #18932. (#19001)
@graouts

This comment has been minimized.

Copy link
Contributor

@graouts graouts commented Sep 11, 2019

I updated the test, I think this closes this issue.

@graouts graouts closed this Sep 11, 2019
@graouts

This comment has been minimized.

Copy link
Contributor

@graouts graouts commented Sep 11, 2019

That's what I like to hear! Next step is to fix this last assertion in Safari.

@birtles

This comment has been minimized.

Copy link
Contributor

@birtles birtles commented Sep 11, 2019

@birtles does it sound right to you to make this change?

The change seems fine. I'm not sure if I agree with having the test harness error on unhandled Promise rejections, however.

@foolip

This comment has been minimized.

Copy link
Contributor Author

@foolip foolip commented Sep 12, 2019

I'm not sure if I agree with having the test harness error on unhandled Promise rejections, however.

This happens here:

wpt/resources/testharness.js

Lines 3368 to 3399 in 152adb0

var error_handler = function(e) {
if (tests.tests.length === 0 && !tests.allow_uncaught_exception) {
tests.set_file_is_test();
}
var stack;
if (e.error && e.error.stack) {
stack = e.error.stack;
} else {
stack = e.filename + ":" + e.lineno + ":" + e.colno;
}
if (tests.file_is_test) {
var test = tests.tests[0];
if (test.phase >= test.phases.HAS_RESULT) {
return;
}
test.set_status(test.FAIL, e.message, stack);
test.phase = test.phases.HAS_RESULT;
// The following function invocation is superfluous.
// TODO: Remove.
test.done();
} else if (!tests.allow_uncaught_exception) {
tests.status.status = tests.status.ERROR;
tests.status.message = e.message;
tests.status.stack = stack;
}
done();
};
addEventListener("error", error_handler, false);
addEventListener("unhandledrejection", function(e){ error_handler(e.reason); }, false);

As you can see, uncaught exceptions are given the same treatment as unhandled rejections. Both can be disabled with setup({allow_uncaught_exception : true}), which by now is a bit of a misnomer.

Is there something you think should change here?

@birtles

This comment has been minimized.

Copy link
Contributor

@birtles birtles commented Sep 12, 2019

Is there something you think should change here?

It's a bit odd because it means the test ends up testing more than it should in this case.

I suppose on balance, though, there are probably more tests that benefit from having the unhandled rejection check than those that are complicated by it, so maybe it's fine.

@foolip

This comment has been minimized.

Copy link
Contributor Author

@foolip foolip commented Sep 12, 2019

Sounds like you would have preferred the fix I suggested in #18932 (comment)?

I'll leave it to the reviewers of this directory to decide what's best here, but if there is anything we can improve on the infra side please let us know. I filed #19021 about this issue being too hard to understand, with a better message I could have spotted the problem and sent a PR directly.

@graouts

This comment has been minimized.

Copy link
Contributor

@graouts graouts commented Sep 12, 2019

Sounds like you would have preferred the fix I suggested in #18932 (comment)?

That wouldn't work, cancel() doesn't return a promise, but rather resets the ready promise on the animation.

Maybe there's a setup API we could call instead to suppress the harness error.

@foolip

This comment has been minimized.

Copy link
Contributor Author

@foolip foolip commented Sep 12, 2019

Uh, make that just promise.catch(() => {}).

@graouts

This comment has been minimized.

Copy link
Contributor

@graouts graouts commented Sep 12, 2019

Yeah, that would work. @birtles, would you prefer this over the patch I made with the extra assertion?

@birtles

This comment has been minimized.

Copy link
Contributor

@birtles birtles commented Sep 12, 2019

Yeah, that would work. @birtles, would you prefer this over the patch I made with the extra assertion?

No, what you have is fine. I was just more concerned about if we have to do this a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.