Bug 11545 - Disable the old testcases do not follow the ECMA-262v3 specification.
Summary: Disable the old testcases do not follow the ECMA-262v3 specification.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-11-08 08:01 PST by digdog
Modified: 2006-12-07 19:34 PST (History)
1 user (show)

See Also:


Attachments
Patch.txt (83.78 KB, patch)
2006-11-08 08:01 PST, digdog
no flags Details | Formatted Diff | Diff
patch.txt (80.19 KB, patch)
2006-11-11 06:19 PST, digdog
no flags Details | Formatted Diff | Diff
patch.txt (61.16 KB, patch)
2006-11-12 09:28 PST, digdog
darin: review+
Details | Formatted Diff | Diff
Patch.txt (64.05 KB, patch)
2006-12-07 19:25 PST, digdog
mrowe: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description digdog 2006-11-08 08:01:03 PST
There are 7 tests in js1_2 testcase didn't respect the ECMA-262v3 specification and provided wrong results. And since JavaScriptCore is not going to implement the legacy behavior of old JS versions, I disable these tests.

Reference:
http://bugs.webkit.org/show_bug.cgi?id=11529
Comment 1 digdog 2006-11-08 08:01:39 PST
Created attachment 11424 [details]
Patch.txt
Comment 2 Maciej Stachowiak 2006-11-10 03:06:21 PST
IT's great to disable the failing tests! However, I suggest adding a comment below each removed test explaining why it is invalid. Also, and more importantly, your new expected results seem to contain new failures. I don't think we want to land that. r- for improper new expected results. It seems to be a date issue, maybe fixed now.
Comment 3 digdog 2006-11-10 06:07:50 PST
File:///WebKit/JavaScriptCore/tests/mozilla/js1_2/String/concat.js has 4 tests:

aString.concat([]);  // js1.2 expected: test string[]
aString.concat([1,2,3]);  // js1.2 expected: test string[1,2,3]
'abcde'.concat([]);  // js1.2 expected: abcde[]
'abcde'.concat([1,2,3]); // js1.2 expected: abcde[1,2,3]

According to ECMA 15.5.4.6, the argument of concat should send to ToString and convert into a string value (not String object). So these arguments will be convert into '' and '1,2,3' under ECMA-262v3, not the js1.2 expected '[]' and '[1,2,3]'

The correct results should be:
test string
test string1,2,3
abcde
abcde1,2,3
Comment 4 digdog 2006-11-10 06:22:59 PST
File:///WebKit/JavaScriptCore/tests/mozilla/js1_2/function/Number.js has one test:

Number([1,2,3]); // js1.2 expected: 3

According to ECMA 9.3, when input type was Object, should call ToPrimitive(input arg, hint Number) first, and than ToNumber() later. However, ToPrimitive() will use [[DefaultValue]](hint) rule when input Type was Object (ECMA 8.6.2.6). So the input [1,2,3] will applied [[DefaultValue]](hint) rule with hint Number, and it looks like this:

toString(valuOf([1,2,3]))  => toString(1,2,3) => '1,2,3'

Than ToNumber('1,2,3') results NaN based on ECMA 9.3.1: If the grammar cannot interpret the 
string as an expansion of StringNumericLiteral, then the result of ToNumber is NaN.
Comment 5 digdog 2006-11-10 06:36:53 PST
File:///WebKit/JavaScriptCore/tests/mozilla/js1_2/function/String.js has 2 tests:

String({p:1}) //js1.2 expected: {p:1}
String([1,2,3]) //js1.2 expect: [1, 2, 3]

According to ECMA 9.8, when input type of String object argument was Object, we should applied ToPrimitive(input arg, hint String) first, and later ToString() . And just like previous one, ToPrimitive() will use [[DefaultValue]](hint) with hint String to convert the input (toString() below uses the rule in ECMA 15.2.4.2):

valueOf(toString({p:1}) => valueOf('[object Object]') => '[object Object]'
valueOf(toString([1,2,3])) => valueOf('1,2,3') => '1,2,3'

And ToString() called after ToPrimitive(), so the correct result would be:

[object Object]
1,2,3
Comment 6 digdog 2006-11-10 06:41:04 PST
"r- for improper new expected results."

Sorry, but what does this mean? I don't get "r-"... 
Comment 7 digdog 2006-11-11 06:19:16 PST
Created attachment 11486 [details]
patch.txt
Comment 8 digdog 2006-11-11 06:19:58 PST
expected result updated.
Comment 9 digdog 2006-11-12 09:11:32 PST
Comment on attachment 11486 [details]
patch.txt

remove patch due non-PST timezone regression appeared
(http://bugs.webkit.org/show_bug.cgi?id=4930)
Comment 10 digdog 2006-11-12 09:28:44 PST
Created attachment 11499 [details]
patch.txt

update expected results.
Comment 11 Darin Adler 2006-12-07 15:36:46 PST
Comment on attachment 11499 [details]
patch.txt

This patch looks fine to me, but I'd prefer to see a comment explaining why these tests are disabled instead of just having the tests commented out without explanation.
Comment 12 Darin Adler 2006-12-07 15:39:06 PST
Comment on attachment 11499 [details]
patch.txt

I'll mark this review+, but it would be even better with the comments.
Comment 13 Mark Rowe (bdash) 2006-12-07 16:30:42 PST
digdog, can you please update your patch with the comments that Darin noted so that it can more easily be landed?
Comment 14 digdog 2006-12-07 19:25:02 PST
Created attachment 11769 [details]
Patch.txt

Add comments
Comment 15 Mark Rowe (bdash) 2006-12-07 19:34:57 PST
Thanks Kevin!  Landed in r18071, and turned allow-tabs on for the three JS files that were touched as they were already heavily tabified.