Rewrite the 'window.onerror' tests.

Rewrite the 'window.onerror' tests.

The tests are currently duplicating work all over the place, and are
difficult to update. This patch tries to bring some consistency to the
way the feature is tested in the hopes of making future improvements
simpler to effectively test.

R=ch.dumez@sisa.samsung.com

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

[Mike West, 2013-07-23 14:26:32]

Hi Christophe! Would you mind taking a look at this proposed rewrite of some of
the window.onerror tests? I'm working on adding stack traces, and got a bit
carried away while reworking the tests...

Thanks!

-mike

[do-not-use, 2013-07-23 14:46:08]

Since we are rewriting those tests, why are we not leveraging the js-test-pre.js
/ js-test-post.js infrastructure?

https://codereview.chromium.org/19962008/diff/2001/LayoutTests/fast/events/re...
File LayoutTests/fast/events/resources/onerror-test.js (right):

https://codereview.chromium.org/19962008/diff/2001/LayoutTests/fast/events/re...
LayoutTests/fast/events/resources/onerror-test.js:1: if (window.testRunner)
Would becomes optional if we used js-test-pre.js

https://codereview.chromium.org/19962008/diff/2001/LayoutTests/fast/events/re...
LayoutTests/fast/events/resources/onerror-test.js:6: function log(message) {
Any reason we are not using js-test-pre.js? We could use
testPassed()/testFailed()/debug() instead of rewriting a log() function.

https://codereview.chromium.org/19962008/diff/2001/LayoutTests/fast/events/re...
File LayoutTests/fast/events/resources/window-onerror.js (left):

https://codereview.chromium.org/19962008/diff/2001/LayoutTests/fast/events/re...
LayoutTests/fast/events/resources/window-onerror.js:1: function throwException()
{
Not sure why this file appears as modified and not removed.

https://codereview.chromium.org/19962008/diff/2001/LayoutTests/fast/events/wi...
File LayoutTests/fast/events/window-onerror-07.html (right):

https://codereview.chromium.org/19962008/diff/2001/LayoutTests/fast/events/wi...
LayoutTests/fast/events/window-onerror-07.html:7: if (this === window)
We could use shouldBe(this, window); if we used js-test-pre.js

[Mike West, 2013-07-23 14:51:53]

https://codereview.chromium.org/19962008/diff/2001/LayoutTests/fast/events/re...
File LayoutTests/fast/events/resources/onerror-test.js (right):

https://codereview.chromium.org/19962008/diff/2001/LayoutTests/fast/events/re...
LayoutTests/fast/events/resources/onerror-test.js:6: function log(message) {
On 2013/07/23 14:46:09, Christophe Dumez wrote:
> Any reason we are not using js-test-pre.js? We could use
> testPassed()/testFailed()/debug() instead of rewriting a log() function.

I thought about it, and decided that the advantage was slim since we're
generally dumping values directly into the expectations file, and only rarely
want to make assertions that valueA is X.

But since you've mentioned it too, it's probably the right thing to do. I'll
take another pass tomorrow.

https://codereview.chromium.org/19962008/diff/2001/LayoutTests/fast/events/re...
File LayoutTests/fast/events/resources/window-onerror.js (left):

https://codereview.chromium.org/19962008/diff/2001/LayoutTests/fast/events/re...
LayoutTests/fast/events/resources/window-onerror.js:1: function throwException()
{
On 2013/07/23 14:46:09, Christophe Dumez wrote:
> Not sure why this file appears as modified and not removed.

Hrm. It's deleted locally. Perhaps Git is confused?

[Mike West, 2013-07-24 12:58:34]

I've reworked things to use js-test-pre/post. It's sometimes helpful. :)

Would you mind taking another look at the test changes?

-mike

[do-not-use, 2013-07-24 13:18:01]

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/r...
File LayoutTests/fast/events/resources/onerror-test.js (right):

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resources/onerror-test.js:10: function
DumpOnErrorArgumentValuesAndReturn(returnValue, callback) {
nit: function usually start with a lowercase in Blink (unlike Chromium).

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resources/onerror-test.js:12: debug("window.onerror: \""
+ message + "\" at " + stripURL(url) + " (" + line + ", " + column + ")");
nit: I find the output in expected results not very readable for the value in
the parenthesis. Maybe we could add "line:" / "column:" before them to clarify?

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resources/onerror-test.js:17: debug("- Returning 'true':
the error should not be reported in the console as an unhandled exception.");
I think we can remove the "- " prefix, I don't find it makes the output more
readable.

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resources/onerror-test.js:24: function
DumpErrorEventAndAllowDefault(callback) {
Ditto.

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resources/onerror-test.js:33: function
DumpErrorEventAndPreventDefault(callback) {
ditto.

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resources/onerror-test.js:45: function DumpErrorEvent(e)
{
Ditto.

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-errorevent-01.html (right):

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-errorevent-01.html:11: <p>This test should
trigger 'window.onerror', and successfully handle the error.</p>
We can use: description("This test should trigger 'window.onerror', and
successfully handle the error.");

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-onerror-05-expected.txt (right):

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-onerror-05-expected.txt:2: FAIL
successfullyParsed should be true (of type boolean). Was undefined (of type
undefined).
FAIL?

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-onerror-06-expected.txt (right):

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-onerror-06-expected.txt:4: FAIL window.onerror
should only be called once in this test.
FAIL?

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-onerror-08-expected.txt (right):

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-onerror-08-expected.txt:6: FAIL
successfullyParsed should be true (of type boolean). Was undefined (of type
undefined).
FAIL?

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-onerror-08.html (right):

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-onerror-08.html:19: // FIXME: Update this once
the event interface has columns.
Good idea.

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-onerror-11.html (right):

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-onerror-11.html:14: if (errorsHandled === 3 &&
window.testRunner)
I believe the window.testRunner check is superfluous.

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-onerror-12.html (right):

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-onerror-12.html:15: if (errorsHandled === 3 &&
window.testRunner)
superfluous testRunner check?

[Mike West, 2013-07-24 15:28:32]

Let's try that one more time, shall we?  :)

Thanks for the speedy reviews, by the way. I'm impressed!

-mike

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/r...
File LayoutTests/fast/events/resources/onerror-test.js (right):

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resources/onerror-test.js:10: function
DumpOnErrorArgumentValuesAndReturn(returnValue, callback) {
On 2013/07/24 13:18:01, Christophe Dumez wrote:
> nit: function usually start with a lowercase in Blink (unlike Chromium).

Done.

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resources/onerror-test.js:12: debug("window.onerror: \""
+ message + "\" at " + stripURL(url) + " (" + line + ", " + column + ")");
On 2013/07/24 13:18:01, Christophe Dumez wrote:
> nit: I find the output in expected results not very readable for the value in
> the parenthesis. Maybe we could add "line:" / "column:" before them to
clarify?

Done.

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resources/onerror-test.js:17: debug("- Returning 'true':
the error should not be reported in the console as an unhandled exception.");
On 2013/07/24 13:18:01, Christophe Dumez wrote:
> I think we can remove the "- " prefix, I don't find it makes the output more
> readable.

Done.

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resources/onerror-test.js:24: function
DumpErrorEventAndAllowDefault(callback) {
On 2013/07/24 13:18:01, Christophe Dumez wrote:
> Ditto.

Done.

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resources/onerror-test.js:33: function
DumpErrorEventAndPreventDefault(callback) {
On 2013/07/24 13:18:01, Christophe Dumez wrote:
> ditto.

Done.

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/r...
LayoutTests/fast/events/resources/onerror-test.js:45: function DumpErrorEvent(e)
{
On 2013/07/24 13:18:01, Christophe Dumez wrote:
> Ditto.

Done.

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-errorevent-01.html (right):

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-errorevent-01.html:11: <p>This test should
trigger 'window.onerror', and successfully handle the error.</p>
On 2013/07/24 13:18:01, Christophe Dumez wrote:
> We can use: description("This test should trigger 'window.onerror', and
> successfully handle the error.");

Done.

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-onerror-05-expected.txt (right):

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-onerror-05-expected.txt:2: FAIL
successfullyParsed should be true (of type boolean). Was undefined (of type
undefined).
On 2013/07/24 13:18:01, Christophe Dumez wrote:
> FAIL?

Fixed in the other CL.

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-onerror-06-expected.txt (right):

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-onerror-06-expected.txt:4: FAIL window.onerror
should only be called once in this test.
On 2013/07/24 13:18:01, Christophe Dumez wrote:
> FAIL?

> 0 should have been > 1. I confused myself. :)

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-onerror-08-expected.txt (right):

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-onerror-08-expected.txt:6: FAIL
successfullyParsed should be true (of type boolean). Was undefined (of type
undefined).
On 2013/07/24 13:18:01, Christophe Dumez wrote:
> FAIL?

Done.

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-onerror-11.html (right):

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-onerror-11.html:14: if (errorsHandled === 3 &&
window.testRunner)
On 2013/07/24 13:18:01, Christophe Dumez wrote:
> I believe the window.testRunner check is superfluous.

Done.

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-onerror-12.html (right):

https://codereview.chromium.org/19962008/diff/10001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-onerror-12.html:15: if (errorsHandled === 3 &&
window.testRunner)
On 2013/07/24 13:18:01, Christophe Dumez wrote:
> superfluous testRunner check?

Done.

[do-not-use, 2013-07-24 15:46:16]

https://codereview.chromium.org/19962008/diff/17001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-errorevent-01.html (right):

https://codereview.chromium.org/19962008/diff/17001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-errorevent-01.html:11: <p>This test should
trigger 'window.onerror', and successfully handle the error.</p>
Maybe I'm misreading the diff but this new test does not seem to have a
corresponding expectation. Also, it looks like this may be a duplicate test.

Also not using description().

https://codereview.chromium.org/19962008/diff/17001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-errorevent-02.html (right):

https://codereview.chromium.org/19962008/diff/17001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-errorevent-02.html:11: <p>This test should
trigger 'window.onerror', and leave the error unhandled.</p>
Duplicate test without expectation?

https://codereview.chromium.org/19962008/diff/17001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-onerror-10.html (right):

https://codereview.chromium.org/19962008/diff/17001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-onerror-10.html:16: if (errorsHandled === 3 &&
window.testRunner)
nit: superfluous window.testRunner check

[Mike West, 2013-07-25 08:09:42]

Thanks. One more take.

https://codereview.chromium.org/19962008/diff/17001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-errorevent-01.html (right):

https://codereview.chromium.org/19962008/diff/17001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-errorevent-01.html:11: <p>This test should
trigger 'window.onerror', and successfully handle the error.</p>
On 2013/07/24 15:46:16, Christophe Dumez wrote:
> Maybe I'm misreading the diff but this new test does not seem to have a
> corresponding expectation. Also, it looks like this may be a duplicate test.
> 
> Also not using description().

Ugh. This expectation was indeed missing. That's fixed in the next iteration,
along with the other bits.

Regarding duplication: this test calls e.preventDefault(), the other test does
not, so you should see an unhandled exception pop up in the results.

https://codereview.chromium.org/19962008/diff/17001/LayoutTests/fast/events/w...
File LayoutTests/fast/events/window-onerror-10.html (right):

https://codereview.chromium.org/19962008/diff/17001/LayoutTests/fast/events/w...
LayoutTests/fast/events/window-onerror-10.html:16: if (errorsHandled === 3 &&
window.testRunner)
On 2013/07/24 15:46:16, Christophe Dumez wrote:
> nit: superfluous window.testRunner check

Done.

[do-not-use, 2013-07-25 08:13:00]

On 2013/07/25 08:09:42, Mike West wrote:
> Thanks. One more take.
> 
>
https://codereview.chromium.org/19962008/diff/17001/LayoutTests/fast/events/w...
> File LayoutTests/fast/events/window-errorevent-01.html (right):
> 
>
https://codereview.chromium.org/19962008/diff/17001/LayoutTests/fast/events/w...
> LayoutTests/fast/events/window-errorevent-01.html:11: <p>This test should
> trigger 'window.onerror', and successfully handle the error.</p>
> On 2013/07/24 15:46:16, Christophe Dumez wrote:
> > Maybe I'm misreading the diff but this new test does not seem to have a
> > corresponding expectation. Also, it looks like this may be a duplicate test.
> > 
> > Also not using description().
> 
> Ugh. This expectation was indeed missing. That's fixed in the next iteration,
> along with the other bits.

I still don't see the expected results for those 2 tests in patch set 5.

[Mike West, 2013-07-25 08:32:11]

I swear I'm not trolling you. Expectations are there this time.

[do-not-use, 2013-07-25 08:34:21]

Ok, LGTM now :)

[commit-bot: I haz the power, 2013-07-25 08:38:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkwst@chromium.org/19962008/32001

[commit-bot: I haz the power, 2013-07-25 09:20:49]

Change committed as 154910