Increase coverage testing of sparse arrays. Add a set...

Increase coverage testing of sparse arrays.  Add a set
of tests to verify that the hole is preserved when shifting
and splicing.

[iposva, 2009-04-08 01:01:18]

Comments below, otherwise LGTM.

-Ivan

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

http://codereview.chromium.org/63100/diff/1/3#newcode29
Line 29: arr = [1, 2, 3, 4, 5];
The fact that the array indices and the values do not line up is rather
confusing. To distinguish indices and values, can you use ["a", "b", "c", "d",
"e"]? Or if you prefer not to use strings then use variables var a = 10; var b =
20; ... arr = [a, b, c, d, e];

In any case it would more clearly express your intentions if you used a
different value range for the array contents at least: arr = [100, 101, 102,
103, 104, 105];

http://codereview.chromium.org/63100/diff/1/3#newcode41
Line 41: assertArrayEquals([2,,4,,], arr);
Do you need the extra comma here before the closing bracket?

http://codereview.chromium.org/63100/diff/1/3#newcode87
Line 87: assertFalse(1999 in arr);
How about also asserting that the indices between 2000 and 2100 are in the array
before the splicing operations? Similarly for the next chunks of missing and
available indices.

http://codereview.chromium.org/63100/diff/1/2
File test/mjsunit/array-splice-webkit.js (right):

http://codereview.chromium.org/63100/diff/1/2#newcode61
Line 61: arr = [1,2,3,4,5];
Same concern about mixing indices and values...

[Mike Belshe, 2009-04-08 15:37:48]

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

http://codereview.chromium.org/63100/diff/1/3#newcode29
Line 29: arr = [1, 2, 3, 4, 5];
On 2009/04/08 01:01:18, iposva wrote:
> The fact that the array indices and the values do not line up is rather
> confusing. To distinguish indices and values, can you use ["a", "b", "c", "d",
> "e"]? Or if you prefer not to use strings then use variables var a = 10; var b
=
> 20; ... arr = [a, b, c, d, e];
> 
> In any case it would more clearly express your intentions if you used a
> different value range for the array contents at least: arr = [100, 101, 102,
> 103, 104, 105];
> 

Done.

http://codereview.chromium.org/63100/diff/1/3#newcode41
Line 41: assertArrayEquals([2,,4,,], arr);
On 2009/04/08 01:01:18, iposva wrote:
> Do you need the extra comma here before the closing bracket?

Yeah, there is a hole here.  The array equals would fail if it were omitted.

http://codereview.chromium.org/63100/diff/1/3#newcode87
Line 87: assertFalse(1999 in arr);
This test is verifying splicing, not that generic array creation/setup works.  I
could add more asserts, but I think it is gratuitous because we have other tests
for that and because this test will still fail (albeit later). 



On 2009/04/08 01:01:18, iposva wrote:
> How about also asserting that the indices between 2000 and 2100 are in the
array
> before the splicing operations? Similarly for the next chunks of missing and
> available indices.

http://codereview.chromium.org/63100/diff/1/2
File test/mjsunit/array-splice-webkit.js (right):

http://codereview.chromium.org/63100/diff/1/2#newcode61
Line 61: arr = [1,2,3,4,5];
On 2009/04/08 01:01:18, iposva wrote:
> Same concern about mixing indices and values...

Done.