Migrating another batch of tests to dart 2.

Migrating another batch of tests to dart 2.

Start:
./language[_strong]/closure_side_effect_test.dart
End:
./language[_strong]/compile_time_constant2_test.dart

Except for tests/language_2/closure_type_test.dart, all other files has been copied automatically by the tool.

R=jcollins@google.com, whesse@google.com

PS: Forgot to run git cl upload command right after the migrate_tool, which I should have done, to make the automatic and manual changes clearer.
Committed: https://github.com/dart-lang/sdk/commit/312ce6d0021e37c9382a8f1a9c27938b1d79c663

[mkroghj, 2017-08-14 13:19:54]

Description was changed from

==========
Migrating another batch of tests to dart 2.

Start:
./language[_strong]/closure_side_effect_test.dart
End:
./language[_strong]/compile_time_constant2_test.dart
==========

to

==========
Migrating another batch of tests to dart 2.

Start:
./language[_strong]/closure_side_effect_test.dart
End:
./language[_strong]/compile_time_constant2_test.dart

Except for tests/language_2/closure_type_test.dart, all other files has been
copied automatically by the tool.
==========

[mkroghj, 2017-08-14 13:21:16]

Description was changed from

==========
Migrating another batch of tests to dart 2.

Start:
./language[_strong]/closure_side_effect_test.dart
End:
./language[_strong]/compile_time_constant2_test.dart

Except for tests/language_2/closure_type_test.dart, all other files has been
copied automatically by the tool.
==========

to

==========
Migrating another batch of tests to dart 2.

Start:
./language[_strong]/closure_side_effect_test.dart
End:
./language[_strong]/compile_time_constant2_test.dart

Except for tests/language_2/closure_type_test.dart, all other files has been
copied automatically by the tool.

PS: Forgot to run git cl upload command right after the migrate_tool, which I
should have done, to make the automatic and manual changes clearer.
==========

[mkroghj, 2017-08-14 13:23:20]

mkroghj@google.com changed reviewers:
+ jcollins@google.com, rnystrom@google.com, whesse@google.com

[mkroghj, 2017-08-14 13:23:21]

ptal

[Bill Hesse, 2017-08-14 13:44:17]

lgtm

Note the only non-trivial change is to closure_type_test, which was testing a
runtime check on a conversion from double to int.  We changed this to classes A
and B extending A.

[jcollins, 2017-08-15 16:02:59]

On 2017/08/14 13:44:17, Bill Hesse wrote:
> lgtm
> 
> Note the only non-trivial change is to closure_type_test, which was testing a
> runtime check on a conversion from double to int.  We changed this to classes
A
> and B extending A.

It looks like this completely changes what the test is testing.  What is the
justification for this?

Janice

[jcollins, 2017-08-15 16:12:00]

On 2017/08/15 16:02:59, jcollins wrote:
> On 2017/08/14 13:44:17, Bill Hesse wrote:
> > lgtm
> > 
> > Note the only non-trivial change is to closure_type_test, which was testing
a
> > runtime check on a conversion from double to int.  We changed this to
classes
> A
> > and B extending A.
> 
> It looks like this completely changes what the test is testing.  What is the
> justification for this?
> 
> Janice

(Read justification on another thread).

I think it's safer to mark the test as not working in dart2js rather than
rewriting the test to test something else.

Janice

[Bill Hesse, 2017-08-15 16:28:04]

whesse@google.com changed reviewers:
+ regis@google.com

[Bill Hesse, 2017-08-15 16:28:26]

Looking at the history of the test, in
https://github.com/dart-lang/sdk/commit/5919998308fcb1ca7b086e47129fc8a07c7f0101
and what it is intended to be testing, it is much better to change that test to
actually test what it claims, in strong mode,
than to just leave it broken in strong mode on browsers.

There is no indication that the test was written to intentionally not work on
browsers, and we can check with the author as well.
It doesn't completely change what the test is testing at all - the dynamic type
error we get, because of dynamic checks introduced
at runtime (originally in checked mode, now in strong mode), is exactly the same
thing, caused the same way.

[regis, 2017-08-15 17:40:45]

On 2017/08/15 16:28:26, Bill Hesse wrote:
> Looking at the history of the test, in
>
https://github.com/dart-lang/sdk/commit/5919998308fcb1ca7b086e47129fc8a07c7f0101
> and what it is intended to be testing, it is much better to change that test
to
> actually test what it claims, in strong mode,
> than to just leave it broken in strong mode on browsers.
> 
> There is no indication that the test was written to intentionally not work on
> browsers, and we can check with the author as well.
> It doesn't completely change what the test is testing at all - the dynamic
type
> error we get, because of dynamic checks introduced
> at runtime (originally in checked mode, now in strong mode), is exactly the
same
> thing, caused the same way.

I agree with Bill and Morten. The rewritten test perfectly preserves the intent
of the original test. The fact that int and double behave differently in
browsers has nothing to do with this test.

I would only remove the line "library closure_type_test;", which is not
necessary anymore (I think).

Cheers,
Regis

[jcollins, 2017-08-15 17:55:24]

On 2017/08/15 17:40:45, regis wrote:
> On 2017/08/15 16:28:26, Bill Hesse wrote:
> > Looking at the history of the test, in
> >
>
https://github.com/dart-lang/sdk/commit/5919998308fcb1ca7b086e47129fc8a07c7f0101
> > and what it is intended to be testing, it is much better to change that test
> to
> > actually test what it claims, in strong mode,
> > than to just leave it broken in strong mode on browsers.
> > 
> > There is no indication that the test was written to intentionally not work
on
> > browsers, and we can check with the author as well.
> > It doesn't completely change what the test is testing at all - the dynamic
> type
> > error we get, because of dynamic checks introduced
> > at runtime (originally in checked mode, now in strong mode), is exactly the
> same
> > thing, caused the same way.
> 
> I agree with Bill and Morten. The rewritten test perfectly preserves the
intent
> of the original test. The fact that int and double behave differently in
> browsers has nothing to do with this test.
> 
> I would only remove the line "library closure_type_test;", which is not
> necessary anymore (I think).
> 
> Cheers,
> Regis

Yes, reviewing the history, I see what's going on here better. Agreed.

LGTM.

[mkroghj, 2017-08-16 09:51:52]

Description was changed from

==========
Migrating another batch of tests to dart 2.

Start:
./language[_strong]/closure_side_effect_test.dart
End:
./language[_strong]/compile_time_constant2_test.dart

Except for tests/language_2/closure_type_test.dart, all other files has been
copied automatically by the tool.

PS: Forgot to run git cl upload command right after the migrate_tool, which I
should have done, to make the automatic and manual changes clearer.
==========

to

==========
Migrating another batch of tests to dart 2.

Start:
./language[_strong]/closure_side_effect_test.dart
End:
./language[_strong]/compile_time_constant2_test.dart

Except for tests/language_2/closure_type_test.dart, all other files has been
copied automatically by the tool.

R=jcollins@google.com, whesse@google.com

PS: Forgot to run git cl upload command right after the migrate_tool, which I
should have done, to make the automatic and manual changes clearer.
Committed:
https://github.com/dart-lang/sdk/commit/312ce6d0021e37c9382a8f1a9c27938b1d79c663
==========

[mkroghj, 2017-08-16 09:51:54]

Committed patchset #2 (id:20001) manually as
312ce6d0021e37c9382a8f1a9c27938b1d79c663.