1019323 - Tests execution continues after calling unexpectedEventAndFinish()

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Nils Ohlmeier [:drno]]

unexpectedEventAndFinish() only results in a test error, but the test continues processing the test chain as if nothing happened.
Instead this should be treated as a fatal error and the test execution should be stopped.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: bug_1019323_exit_test_after_unexpectedEventAndFinish.patch

Comment 2

[Nils Ohlmeier [:drno]]

Attachment: bug_1019323_exit_test_after_unexpectedEventAndFinish.patch

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: bug_1019323_convert_ok_false.patch

Try run: https://tbpl.mozilla.org/?tree=Try&rev=2292f29857a1

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8432954 [details] [diff] [review]
bug_1019323_exit_test_after_unexpectedEventAndFinish.patch

Review of attachment 8432954 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the mentioned unclear code clarified.

::: dom/media/tests/mochitest/pc.js
@@ +89,5 @@
> +    // and shutdown the test
> +    if ((typeof SimpleTest._alreadyFinished !== "undefined") &&
> +      (SimpleTest._alreadyFinished == true)) {
> +      self.finished = true;
> +    }

I don't understand why we need that code here. If SimpleTest.finish() has been called, the code here won't be executed at all, or? I think it would be good to also request review from Ted here. I'm not that familiar with this specific behavior. Otherwise the patch looks fine.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8432955 [details] [diff] [review]
bug_1019323_convert_ok_false.patch

Review of attachment 8432955 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not familiar with all the latest additions to the webrtc tests and libs. I don't have the time to dig into this right now. So best is to ask Jesup for review.

Comment 6

[Nils Ohlmeier [:drno]]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> I don't understand why we need that code here. If SimpleTest.finish() has
> been called, the code here won't be executed at all, or? I think it would be
> good to also request review from Ted here. I'm not that familiar with this
> specific behavior. Otherwise the patch looks fine.

Unfortunately that is not the case. As long as there are still steps in the PeerConnectionWrapper.chain the test continuous until the chain is empty. This what this parts implements: ignore the rest of the chain if SimpleTest.finish() has been called, because that seems to be the common expectation of that function.

Comment 7

[Nils Ohlmeier [:drno]]

Ted: since I wrote this patch I actually noticed that the problem of not exiting seems to exist on desktop/local builds. On TBPL SimpleTest.finish() seems to terminate the test case properly while it is running. Do you know any reason for that difference?

I have stuff depending on this: so could you please review or nominate someone else?

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 8432954 [details] [diff] [review]
bug_1019323_exit_test_after_unexpectedEventAndFinish.patch

Review of attachment 8432954 [details] [diff] [review]:
-----------------------------------------------------------------

The only thing I'm worried about here is that we might still have a little bit of a race between calling SimpleTest.finish(), the Mochitest harness moving on to the next test, and pc.js running "self.onFinished()".

Would it make sense to remove the calls to SimpleTest.finish(), and replace them with a CommandChain.finish() that set .finish(), and call SimpleTest.finish in one place, in an else block of the "if (!self.finished) {" in _executeNext?

Comment 9

[(not currently active) Ted Mielczarek]

(In reply to Nils Ohlmeier [:drno] from comment #7)
> Ted: since I wrote this patch I actually noticed that the problem of not
> exiting seems to exist on desktop/local builds. On TBPL SimpleTest.finish()
> seems to terminate the test case properly while it is running. Do you know
> any reason for that difference?

Are you running a single test, or a directory of tests? The Mochitest harness does different things in those cases.

Comment 10

[Nils Ohlmeier [:drno]]

This should be solved by the re-write with Promises which exit the test after any error.