JS Test Harness: Make successfullyParsed optional

[Erik Arvidsson]

JS Test Harness: Make successfullyParsed optional

[Erik Arvidsson]

Created attachment 112284 [details]
Patch

[Ojan Vafai]

Comment on attachment 112284 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112284&action=review

This is awesome!

> LayoutTests/fast/js/kde/garbage-n.html:13
> +shouldBeTrue("hadSyntaxError");
> +hadSyntaxError = false;

As discussed in person, js-test-pre should just have a function like the following that is called instead so that hadSyntaxError can be private and so that people don't have to understand the guts of js-test-pre as much.

function shouldHaveHadSyntaxError()
{
    shouldBeTrue("hadSyntaxError");
    hadSyntaxError = false;
}

I'm fine with having a FIXME in js-test-pre and doing it in a followup cleanup patch.

> LayoutTests/fast/js/resources/js-test-post-async.js:2
> +  successfullyParsed = true;

needs 4 space indent

> LayoutTests/fast/js/resources/js-test-pre.js:393
> +    // FIXME: Remove

A little more detail would be nice. "Remove once ...".

> LayoutTests/fast/multicol/column-span-parent-continuation-crash.html:13
> +        layoutTestController.waitUntilDone();

this is indented too far now.

> LayoutTests/java/lc3/Constructors/construct-001-expected.txt:5
>  
> -FAIL successfullyParsed should be true. Threw exception ReferenceError: Can't find variable: successfullyParsed
> +PASS successfullyParsed is true
>  

Can you add to the ChangeLog description why this new result is correct?

[Alexey Proskuryakov]

One good thing about our tests is that they can generally run in any browser. It's very important to compare behavior, and it's sometimes helpful to send a trac.webkit.org link to another browser's developer to illustrate a point.

Can relying on window.onerror make this less reliable?

Also, when changing JS tests, please also change make-new-script-test script.

[Erik Arvidsson]

(In reply to comment #3)
> One good thing about our tests is that they can generally run in any browser. It's very important to compare behavior, and it's sometimes helpful to send a trac.webkit.org link to another browser's developer to illustrate a point.

Understood. WebKit was one of the last browser to get support for window.onerror so this should not be an issue unless we want to ensure this works in old versions of Opera?

> Can relying on window.onerror make this less reliable?

No, it should make things more reliable if anything. It will capture errors in all script tags, whereas "successfullyParsed = true" would only apply to the script tag that had the assignment expression statement in it.

> Also, when changing JS tests, please also change make-new-script-test script.

Thanks for the pointer. I've updated the script.

[Alexey Proskuryakov]

> WebKit was one of the last browser to get support for window.onerror so this should not be an issue

Thank you, sounds good to me.

[Erik Arvidsson]

Committed r98407: <http://trac.webkit.org/changeset/98407>

[Ryosuke Niwa]

It appears that this patch caused window-property-descriptors.html to fail at least on SL:
http://build.webkit.org/results/SnowLeopard%20Intel%20Debug%20(Tests)/r98427%20(2715)/fast/dom/Window/window-property-descriptors-pretty-diff.html

blame list: http://trac.webkit.org/log/?verbose=on&rev=98412&stop_rev=98403

[Ryosuke Niwa]

Qt: http://build.webkit.org/results/Qt%20Linux%20Release/r98431%20(38958)/fast/dom/Window/window-property-descriptors-pretty-diff.html

Windows:
http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r98431%20(17324)/fast/dom/Window/window-property-descriptors-pretty-diff.html

[Ryosuke Niwa]

Rebaselined in http://trac.webkit.org/changeset/98436 for now since the diff didn't look wrong.