Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(37)

Issue 596116: Elaborating test for Array.shift a bit. (Closed)

Created:
10 years, 10 months ago by antonm
Modified:
9 years, 4 months ago
CC:
v8-dev
Visibility:
Public.

Description

Elaborating test for Array.shift a bit. Committed: http://code.google.com/p/v8/source/detail?r=3855

Patch Set 1 #

Total comments: 2

Patch Set 2 : putting deletions into assertTrue #

Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -4 lines) Patch
M test/mjsunit/array-shift.js View 1 1 chunk +29 lines, -4 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
antonm
10 years, 10 months ago (2010-02-15 11:31:00 UTC) #1
Mads Ager (chromium)
LGTM, thanks! http://codereview.chromium.org/596116/diff/1/2 File test/mjsunit/array-shift.js (right): http://codereview.chromium.org/596116/diff/1/2#newcode68 test/mjsunit/array-shift.js:68: delete Array.prototype[3]; Why do you have these ...
10 years, 10 months ago (2010-02-15 11:34:02 UTC) #2
antonm
10 years, 10 months ago (2010-02-15 12:01:23 UTC) #3
Thanks a lot for review, Mads, committing.

http://codereview.chromium.org/596116/diff/1/2
File test/mjsunit/array-shift.js (right):

http://codereview.chromium.org/596116/diff/1/2#newcode68
test/mjsunit/array-shift.js:68: delete Array.prototype[3];
On 2010/02/15 11:34:02, Mads Ager wrote:
> Why do you have these deletes here?  If you want them, you should test if they
> return true or false.

I don't want to pollute Array.prototype object in case we add more tests. 
Wrapped into assertTrue's.

Powered by Google App Engine
This is Rietveld 408576698