Test for consistent JSArray indexer errors

Test for consistent JSArray indexer errors

Optimized and unoptimized paths should generate the same errors.

R=lrn@google.com

Committed: https://github.com/dart-lang/sdk/commit/ce350d7ea3ed3cbab7ad822e530e7b1636c9b446

[sra1, 2015-09-10 22:10:18]

sra@google.com changed reviewers:
+ lrn@google.com

[sra1, 2015-09-10 22:10:18]

Lasse: some tests to help with changes like your recent improvements.

[Lasse Reichstein Nielsen, 2015-09-11 06:16:58]

lgtm with comments.

https://codereview.chromium.org/1327423002/diff/1/tests/compiler/dart2js_extr...
File tests/compiler/dart2js_extra/js_array_index_error_test.dart (right):

https://codereview.chromium.org/1327423002/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/js_array_index_error_test.dart:17:
Expect.fail('must throw');
Move this outside the try/catch, otherwise you'll just catch the failure
exception.

https://codereview.chromium.org/1327423002/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/js_array_index_error_test.dart:24: fault(i) {
You could write this as:
  fault(i) => () => confuse([])[i];
Then you don't have to add the "() =>" below.
(I'd normally name such a function "mkFault" or "makeFault").

https://codereview.chromium.org/1327423002/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/js_array_index_error_test.dart:84: HUGE =
1000000000000;
Why not initialize HUGE above?
If there is a reason, document it.

https://codereview.chromium.org/1327423002/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/js_array_index_error_test.dart:119: }
Could these tests be parameterized with (list, index), making indexEmpty and
indexNonempty not duplicate as much code, and the Huge tests would just be the
same code too.

In that case I'd pass a name as well to describe the test and use that in the
expects.

https://codereview.chromium.org/1327423002/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/js_array_index_error_test.dart:180:
indexSetNonempty() {
No Huge version of indexSet tests?

https://codereview.chromium.org/1327423002/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/js_array_index_error_test.dart:219:
indexSetNonempty();
Maybe also test negative indices.

[sra1, 2015-09-11 17:27:11]

Patchset #2 (id:20001) has been deleted

[sra1, 2015-09-11 17:33:11]

Committed patchset #2 (id:40001) manually as
ce350d7ea3ed3cbab7ad822e530e7b1636c9b446 (presubmit successful).

[sra1, 2015-09-11 17:33:34]

https://codereview.chromium.org/1327423002/diff/1/tests/compiler/dart2js_extr...
File tests/compiler/dart2js_extra/js_array_index_error_test.dart (right):

https://codereview.chromium.org/1327423002/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/js_array_index_error_test.dart:17:
Expect.fail('must throw');
On 2015/09/11 06:16:57, Lasse Reichstein Nielsen wrote:
> Move this outside the try/catch, otherwise you'll just catch the failure
> exception.

Done.

https://codereview.chromium.org/1327423002/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/js_array_index_error_test.dart:24: fault(i) {
On 2015/09/11 06:16:58, Lasse Reichstein Nielsen wrote:
> You could write this as:
>   fault(i) => () => confuse([])[i];
> Then you don't have to add the "() =>" below.
> (I'd normally name such a function "mkFault" or "makeFault").

Done.

https://codereview.chromium.org/1327423002/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/js_array_index_error_test.dart:84: HUGE =
1000000000000;
On 2015/09/11 06:16:57, Lasse Reichstein Nielsen wrote:
> Why not initialize HUGE above?
> If there is a reason, document it.

Done.

https://codereview.chromium.org/1327423002/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/js_array_index_error_test.dart:119: }
On 2015/09/11 06:16:57, Lasse Reichstein Nielsen wrote:
> Could these tests be parameterized with (list, index), making indexEmpty and
> indexNonempty not duplicate as much code, and the Huge tests would just be the
> same code too.
> 
> In that case I'd pass a name as well to describe the test and use that in the
> expects.

I'm wary that making the code general will inhibit optimizations.

This is a test for getting the same error from optimized and non-optimized code.
Making 'list' a parameter would mean there is one version of the code, so no
difference.

I renamed all these tests to 'constantXXX' since the index expressions are
constant and added some 'variableXXXX' tests where the index is passed in.

https://codereview.chromium.org/1327423002/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/js_array_index_error_test.dart:180:
indexSetNonempty() {
On 2015/09/11 06:16:57, Lasse Reichstein Nielsen wrote:
> No Huge version of indexSet tests?

Added in the 'variable' tests.

https://codereview.chromium.org/1327423002/diff/1/tests/compiler/dart2js_extr...
tests/compiler/dart2js_extra/js_array_index_error_test.dart:219:
indexSetNonempty();
On 2015/09/11 06:16:57, Lasse Reichstein Nielsen wrote:
> Maybe also test negative indices.

Done.