LayoutTests/resources/js-test.js onerror tweak.

LayoutTests/resources/js-test.js onerror tweak.

Previously, the onerror handler set up by js-test.js in LayoutTests did
not call finishJSTest(). If an error was thrown in an async test,
testRunner.notifyDone() was never called, causing a timeout.

This change makes async tests terminate immediately when an unexpected
error occurs, making it easier/faster to debug async tests.

Tests that expect unhandled errors to be thrown (usually signaled by calling
shouldHaveHadError()) must now call expectError() before the errors will
be thrown. This change updates existing tests that use
shouldHaveHadErrors().

BUG=164933
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161860

[pwnall-personal, 2013-11-08 07:19:31]

Can you please take a look?

This is a little tweak to js-test.js that makes development with async tests a
bit faster. The test fails with a timeout without the fix, and passes
afterwards.

[ojan, 2013-11-08 18:37:23]

I like the direction this change is going. Unfortunately, this doesn't quite
work as is because some tests are specifically testing for an error and now
we'll be skipping their asserts.

See
https://code.google.com/p/chromium/codesearch#search/&q=shouldHaveHadError&sq....

One way to fix that along with this patch would be to change shouldHaveHadError
to expectError or something and you call that *before* the error is supposed to
occur. Then, in the onerror handler, you only call finishJSTest if there isn't
an expected error (if there is one, then you call shouldHaveHadError and clear
the expected error).

[pwnall-personal, 2013-11-10 05:21:18]

On 2013/11/08 18:37:23, ojan wrote:
> I like the direction this change is going. Unfortunately, this doesn't quite
> work as is because some tests are specifically testing for an error and now
> we'll be skipping their asserts.
> 
> See
>
https://code.google.com/p/chromium/codesearch#search/&q=shouldHaveHadError&sq....
> 
> One way to fix that along with this patch would be to change
shouldHaveHadError
> to expectError or something and you call that *before* the error is supposed
to
> occur. Then, in the onerror handler, you only call finishJSTest if there isn't
> an expected error (if there is one, then you call shouldHaveHadError and clear
> the expected error).

Thank you very much for your help!

I bumped into some failing tests, but my method of fixing them wasn't anywhere
nearly as clean and elegant as your suggestion! I implemented it and updated
tests.

Can you please take another look and see if I understood you correctly?

[ojan, 2013-11-11 20:22:29]

https://codereview.chromium.org/59903016/diff/220001/LayoutTests/resources/js...
File LayoutTests/resources/js-test.js (right):

https://codereview.chromium.org/59903016/diff/220001/LayoutTests/resources/js...
LayoutTests/resources/js-test.js:109: setTimeout(finishJSTest, 0);
Why the setTimeout?

[pwnall-personal, 2013-11-11 22:45:33]

https://codereview.chromium.org/59903016/diff/220001/LayoutTests/resources/js...
File LayoutTests/resources/js-test.js (right):

https://codereview.chromium.org/59903016/diff/220001/LayoutTests/resources/js...
LayoutTests/resources/js-test.js:109: setTimeout(finishJSTest, 0);
On 2013/11/11 20:22:29, ojan wrote:
> Why the setTimeout?

As I was debugging failed tests on the last patch, I saw that when the
setTimeout is in place, I get the ERROR messages from the console in the text
output. If I call finishJSTest right away, I don't. I like all the help I can
get in debugging my failures, so I thought this is useful.

OTOH, I'm pretty new to the codebase, and I haven't looked at the test shell. I
can get rid of setTimeout if you don't like it.

[ojan, 2013-11-11 23:30:41]

https://codereview.chromium.org/59903016/diff/220001/LayoutTests/resources/js...
File LayoutTests/resources/js-test.js (right):

https://codereview.chromium.org/59903016/diff/220001/LayoutTests/resources/js...
LayoutTests/resources/js-test.js:109: setTimeout(finishJSTest, 0);
On 2013/11/11 22:45:34, pwnall wrote:
> On 2013/11/11 20:22:29, ojan wrote:
> > Why the setTimeout?
> 
> As I was debugging failed tests on the last patch, I saw that when the
> setTimeout is in place, I get the ERROR messages from the console in the text
> output. If I call finishJSTest right away, I don't. I like all the help I can
> get in debugging my failures, so I thought this is useful.

Don't you get the same error message from the testFailed call one line up?

> OTOH, I'm pretty new to the codebase, and I haven't looked at the test shell.
I
> can get rid of setTimeout if you don't like it.

setTimeout often leads to test flakiness. We try to avoid it whenever possible.

[pwnall-personal, 2013-11-12 10:19:56]

On 2013/11/11 23:30:41, ojan wrote:
> setTimeout often leads to test flakiness. We try to avoid it whenever
possible.

I removed the setTimeout call. I fear that it'll lead to more flakiness, because
of disappearing console output. On the other hand, no checked in expectation
should use the codepath that calls finishJSTest(), so I hope this won't matter.
Here's the console output line that disappeared:
https://codereview.chromium.org/59903016/diff2/220001:270003/LayoutTests/fast...

I also changed expected-error-in-async-test so it doesn't rely on timer values.
I hope this reduces the probability of it becoming a flake. 

What do you think?

[pwnall-personal, 2013-11-12 10:22:08]

On 2013/11/12 10:19:56, pwnall wrote:
> On 2013/11/11 23:30:41, ojan wrote:
> > setTimeout often leads to test flakiness. We try to avoid it whenever
> possible.
> 
> I removed the setTimeout call. I fear that it'll lead to more flakiness,
because
> of disappearing console output. On the other hand, no checked in expectation
> should use the codepath that calls finishJSTest(), so I hope this won't
matter.
> Here's the console output line that disappeared:
>
https://codereview.chromium.org/59903016/diff2/220001:270003/LayoutTests/fast...
> 
> I also changed expected-error-in-async-test so it doesn't rely on timer
values.
> I hope this reduces the probability of it becoming a flake. 
> 
> What do you think?

Also, what do you think about checking for console.error and calling it with the
error message before calling finishJSTest()? I don't want to do complex DOM
operations from the onerror handler, but I think a console.error call should be
alright.

[ojan, 2013-11-12 22:04:52]

lgtm

> Also, what do you think about checking for console.error and calling it with
the
> error message before calling finishJSTest()? I don't want to do complex DOM
> operations from the onerror handler, but I think a console.error call should
be
> alright.

Sorry, I don't understand what you're asking here. The patch looks fine as is
IMO if you want to commit this.

[pwnall-personal, 2013-11-12 22:06:06]

On 2013/11/12 22:04:52, ojan wrote:
> lgtm
> 
> > Also, what do you think about checking for console.error and calling it with
> the
> > error message before calling finishJSTest()? I don't want to do complex DOM
> > operations from the onerror handler, but I think a console.error call should
> be
> > alright.
> 
> Sorry, I don't understand what you're asking here. The patch looks fine as is
> IMO if you want to commit this.

I'm even worse at words than at coding, so I'll send off a new patch after this
one gets through the CQ. Sorry for being confusing!

[commit-bot: I haz the power, 2013-11-12 22:41:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/costan@gmail.com/59903016/270003

[commit-bot: I haz the power, 2013-11-12 23:59:58]

Change committed as 161860