Bug 80822

Summary: Tests for RegExp multiline failed
Product: WebKit Reporter: Hojong Han <hojong.han>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, ggaren, oliver, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch none

Description Hojong Han 2012-03-12 01:24:40 PDT
Testcase js1_2/regexp/RegExp_multiline.js failed 
Failure messages were:
(multiline == true) '123\n456'.match(/^4../) = null FAILED! expected: 456 
(multiline == true) 'a11\na22\na23\na24'.match(/^a../g) = a11 FAILED! expected: a11,a22,a23,a24 
(multiline == true) '123\n456'.match(/.3$/) = null FAILED! expected: 23 
(multiline == true) 'a11\na22\na23\na24'.match(/a..$/g) = a24 FAILED! expected: a11,a22,a23,a24 
(multiline == true) 'a11\na22\na23\na24'.match(new RegExp('a..$','g')) = a24 FAILED! expected: a11,a22,a23,a24 

Testcase js1_2/regexp/RegExp_multiline_as_array.js failed 
Failure messages were:
(['$*'] == true) '123\n456'.match(/^4../) = null FAILED! expected: 456 
(['$*'] == true) 'a11\na22\na23\na24'.match(/^a../g) = a11 FAILED! expected: a11,a22,a23,a24 
(['$*'] == true) '123\n456'.match(/.3$/) = null FAILED! expected: 23 
(['$*'] == true) 'a11\na22\na23\na24'.match(/a..$/g) = a24 FAILED! expected: a11,a22,a23,a24 
(['$*'] == true) 'a11\na22\na23\na24'.match(new RegExp('a..$','g')) = a24 FAILED! expected: a11,a22,a23,a24 

Testcase js1_2/regexp/beginLine.js failed 
Failure messages were:
123xyz'.match(new RegExp('^\d+')) = null FAILED! expected: 123 

Testcase js1_2/regexp/endLine.js failed 
Failure messages were:
xyz'.match(new RegExp('\d+$')) = null FAILED! expected: 890

These failed testcases are caused by creating RegExp instances without multiline flag, 'm'.
In these testcases,"RegExp.multiline = true" is used to set multiline flag. It's not in spec as I know of.
Fix testcases or JSC, otherwise just let things untouched. Which one is right?
Comment 1 Gavin Barraclough 2012-03-12 01:47:54 PDT
RegExp.multiline is a read-only property (see ES5.1 15.10.7.4), so these are error in the test case.  For these tests, probably best to change them to postfix the 'm' flag, if that will work.

This reminds me, I should check that strict-mode assignment to multiline etc throws. :-)
Comment 2 Hojong Han 2012-03-12 02:07:54 PDT
Created attachment 131298 [details]
Patch
Comment 3 WebKit Review Bot 2012-03-12 02:11:19 PDT
Attachment 131298 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline_as_array.js:87:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline_as_array.js:91:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline_as_array.js:99:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline_as_array.js:103:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline_as_array.js:107:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/endLine.js:64:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline.js:87:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline.js:91:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline.js:99:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline.js:103:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/RegExp_multiline.js:107:  Line contains tab character.  [whitespace/tab] [5]
Source/JavaScriptCore/tests/mozilla/js1_2/regexp/beginLine.js:64:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 12 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Gavin Barraclough 2012-03-12 10:05:57 PDT
Comment on attachment 131298 [details]
Patch

Patch looks great, but we do require just spaces, not tab characters.  Please fix the style issue, but the change looks fine.
Comment 5 Geoffrey Garen 2012-03-12 12:19:58 PDT
> In these testcases,"RegExp.multiline = true" is used to set multiline flag. It's not in spec as I know of.
> Fix testcases or JSC, otherwise just let things untouched. Which one is right?

I'd suggest testing in other modern browsers and seeing what they do.
Comment 6 Gavin Barraclough 2012-03-12 12:25:36 PDT
(In reply to comment #5)
> > In these testcases,"RegExp.multiline = true" is used to set multiline flag. It's not in spec as I know of.
> > Fix testcases or JSC, otherwise just let things untouched. Which one is right?
> 
> I'd suggest testing in other modern browsers and seeing what they do.

Just to clarify, the comment re not being in the spec is in error; behavior here is well defined (and our implementation is correct).
Comment 7 Hojong Han 2012-03-12 21:52:55 PDT
Created attachment 131540 [details]
Patch
Comment 8 Gavin Barraclough 2012-03-12 22:56:31 PDT
Comment on attachment 131540 [details]
Patch

Thanks for the fix - looks great!
Comment 9 WebKit Review Bot 2012-03-12 23:16:15 PDT
Comment on attachment 131540 [details]
Patch

Clearing flags on attachment: 131540

Committed r110540: <http://trac.webkit.org/changeset/110540>
Comment 10 WebKit Review Bot 2012-03-12 23:16:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Hojong Han 2012-03-12 23:49:46 PDT
(In reply to comment #8)
> (From update of attachment 131540 [details])
> Thanks for the fix - looks great!
Thank you for review