Skip to content
This repository has been archived by the owner on Sep 19, 2018. It is now read-only.

EventWatcher: reject waitingFor promise during test cleanup. #225

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pwnall
Copy link
Contributor

@pwnall pwnall commented Dec 13, 2016

EventWatcher installs a test cleanup function that removes its event listeners. For this reason, if the EventWatcher's wait_for created a promise that is still pending when the test ends, the promise will never be resolved or rejected.

This omission turns into a problem when the EventWatcher's wait_for promise ends up in the promise chain used by promise_test. If an assertion fails in a step() callback, the test's done() method is called, which cleans up the EventWatcher's listener. So, a failure in a single promise_test can block the whole promise chain and prevent all subsequent tests from running.

This commit fixes the issue described above by having any pending EventWatcher promise get rejected when the event listeners get removed.

Test case below. If assert_true(false is replaced with assert_true(true, both tests pass cleanly. With this PR, the first test fails and the second test passes. Without the PR, the first test times out and the second test never gets to run.

<!doctype html>
<script src="testharness.js"></script>
<script src="testharnessreport.js"></script>
<script>

promise_test(t => new Promise((resolve, reject) => {
  const waiter = (new EventWatcher(t, window, 'message')).wait_for('message');

  t.step(() => assert_true(false, 'AssertionError in Test.step() callback'));

  window.postMessage('Move to the next test', '*');
  resolve(waiter);
}), 'First promise test');

promise_test(() => Promise.resolve(), 'Second promise test');

</script>

This change is Reviewable

EventWatcher installs a test cleanup function that removes its event
listeners. For this reason, if the EventWatcher's wait_for created a
promise that is still pending when the test ends, the promise will never
be resolved or rejected.

This omission turns into a problem when the EventWatcher's wait_for
promise ends up in the promise chain used by promise_test. If an
assertion fails in a step() callback, the test's done() method is
called, which cleans up the EventWatcher's listener. So, a failure in a
single promise_test can block the whole promise chain and prevent all
subsequent tests from running.

This commit fixes the issue described above by having any pending EventWatcher
promise get rejected when the event listeners get removed.
@mkruisselbrink
Copy link
Contributor

So does this fix #187?

@pwnall
Copy link
Contributor Author

pwnall commented Dec 13, 2016

I think that my patch guarantees that EventWatcher's promise will resolve/reject, as long as the event fires. I suspect that it will solve the situation that #187 ran into, but I can't be sure without seeing a code snippet. Specifically, I haven't thought about the impact on assertion failures in promises (where the promise would get rejected) or in errors that aren't wrapped in a Test.step() call.

Could we ask #187 for more input?

@mkruisselbrink
Copy link
Contributor

I don't think you should worry about errors that aren't wrapped in a Test.step() call (and that don't ultimately result in the promise passed to promise_test to reject). But it would be nice if the solution would work for any assertion failure wrapped in Test.step().
I think this looks reasonable, but as mentioned in #187 the problem is more general than just this EventWatcher case. I wonder if it makes sense to somehow change promise_test to detect these cases and continue with the next test.

Maybe Test can have a promise that is resolved/rejected when the test passes/fails, and promise_test can then race that promise with the promise passed to promise_test? That way you'll catch any situation where an assertion fails outside of the promise chain in a promise_test. Haven't thought about any unintended consequences too much, but it seems that running more tests, even if later results might be unreliable would still be preferred over just not running more tests at all.

@pwnall
Copy link
Contributor Author

pwnall commented Dec 13, 2016

My fix will definitely work for errors wrapped in Test.step(), because the catch clause in step() calls Test.done().

I think that my PR is orthogonal to the fix that you're proposing. I think that EventWatcher's promises should resolve/reject as predictably as possible, to facilitate debugging. I can work on a PR with the fix that you're proposing :)

@pwnall
Copy link
Contributor Author

pwnall commented Dec 13, 2016

I created PR #226 with the fix that you proposed above.

@mkruisselbrink
Copy link
Contributor

But with that fix, would purpose does this PR still serve? The only observable differences I can think of would be in cases where EventWatcher isn't used in a promise_test. But in that case I'm not convinced that rejecting the promise in all cases when the test is done is the right thing to do.

@pwnall
Copy link
Contributor Author

pwnall commented Dec 13, 2016

The API for EventWatcher says wait_for. I was surprised that it stops waiting when a test is done, and I was further surprised that a test would be done the moment an assertion fails. Given that EventWatcher does something that's not obvious from the API name, I think it makes sense to reject the promise. As a developer, this could help me debug code where I incorrectly use an EventWatcher.

From a different angle: wait_for rejects if it receives an event whose type doesn't match the expected event type. It seems like EventWatcher implicitly adds an "end of test" event to the list of events given to the constructor. So, I think it'd make sense that when the "end of test" event occurs, the wait_for promise rejects.

WDYT?

@mkruisselbrink
Copy link
Contributor

From a different angle: wait_for rejects if it receives an event whose type doesn't match the expected event type.

I don't think it actually does, and I agree that that seems like a weird bug. wait_for never rejects. Instead when it receives an event that doesn't match the expected type it'll have an assertion failure (and without either of the fixes than no other promise tests would get run). I definitely think it makes sense for wait_for to be fixed to reject when one of the EventWatcher assertions failed. I'm still not convinced that it makes sense to reject when any other (possibly unrelated) assertion fails.

I almost wonder if the whole stop_watching shouldn't be there, and instead EventWatcher should be a pure Promise based thing that either resolves or rejects, but doesn't actually effect a Test instance. But that might lead to people forgetting to check for promise rejection when used outside of promise_test. So probably a bad idea too. And for what it's worth, the "stop all test code from running when the first assertion failed" logic that is build in to Test.step would already have the effect of no more assertion failures and/or promise resolving after done() is called (step becomes a no-op after the test finished).

@jevery23
Copy link

I have hit an issue with eventWatcher, it doesn't seem to detect taps sometimes, but sticking a timeout in the test after wait_for call of about 5 seconds allows the eventWatcher to settle and detect all taps. Also it does not reject when the event being looked for is not thrown, this may be by design, but a reject with message is preferred.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants