Migrate test block 7 to Dart 2.0.

Migrate test block 7 to Dart 2.0.

duration2_test.dart converted to a static-only test and skipped where appropriate for non-strong.  error_stack_trace_test converted to a multitest and error_stack_trace1_test modified to use more modern catch convention for stack traces.

R=bkonyi@google.com, rnystrom@google.com

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

[jcollins, 2017-07-25 04:12:55]

Description was changed from

==========
Migrate test block 7 to Dart 2.0.

BUG=
==========

to

==========
Migrate test block 7 to Dart 2.0.

duration2_test.dart converted to a static-only test and skipped where
appropriate for non-strong.  error_stack_trace_test converted to a multitest and
error_stack_trace1_test modified to use more modern catch convention for stack
traces.

BUG=
==========

[jcollins, 2017-07-25 04:12:55]

jcollins@google.com changed reviewers:
+ bkonyi@google.com, rnystrom@google.com

[jcollins, 2017-07-25 04:12:55]

jcollins@google.com changed required reviewers:
+ rnystrom@google.com

[bkonyi, 2017-07-25 15:35:54]

LGTM

[kevmoo, 2017-07-25 19:19:40]

Description was changed from

==========
Migrate test block 7 to Dart 2.0.

duration2_test.dart converted to a static-only test and skipped where
appropriate for non-strong.  error_stack_trace_test converted to a multitest and
error_stack_trace1_test modified to use more modern catch convention for stack
traces.

BUG=
==========

to

==========
Migrate test block 8 to Dart 2.0.

duration2_test.dart converted to a static-only test and skipped where
appropriate for non-strong.  error_stack_trace_test converted to a multitest and
error_stack_trace1_test modified to use more modern catch convention for stack
traces.
==========

[kevmoo, 2017-07-25 19:23:40]

Description was changed from

==========
Migrate test block 8 to Dart 2.0.

duration2_test.dart converted to a static-only test and skipped where
appropriate for non-strong.  error_stack_trace_test converted to a multitest and
error_stack_trace1_test modified to use more modern catch convention for stack
traces.
==========

to

==========
Migrate test block 7 to Dart 2.0.

duration2_test.dart converted to a static-only test and skipped where
appropriate for non-strong.  error_stack_trace_test converted to a multitest and
error_stack_trace1_test modified to use more modern catch convention for stack
traces.
==========

[Bob Nystrom, 2017-07-25 22:05:44]

Lots of feedback, but that's good. It means we are making real progress towards
actually testing Dart 2.0 and not 1.0.

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/corelib...
File tests/corelib_2/corelib_2.status (right):

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/corelib...
tests/corelib_2/corelib_2.status:25: duration2_test: Skip # Static verification
only
I think we can just delete this test. It's validating what happens if you pass a
string to new Duration(), but that's a type error now. I don't think we need to
keep it around for validating the static error since there's nothing
particularly interesting about this particular static error.

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
File tests/corelib_2/error_stack_trace_test.dart (right):

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:10: 
Oof, this test is complicated. I believe the intent here is to cause the runtime
to throw one of each of the various exception classes that extend Error. Then,
for each of those, validate that it has a .stackTrace property that returns a
non-null stack trace.

Some of these errors will disappear entirely in 2.0 because they are reported
statically an no longer need to be thrown at runtime. For those, I think you can
delete the code in question. Others can still be thrown randomly, but you might
need some explicit "dynamic" to avoid hitting a static error first. The rest
should be fine as-is.

I don't think you'll need to split out the static errors into a separate
multitest.

In more detail...

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:12: (499).doesNotExist(); //#
static: compile-time error
This is still a valid runtime error, but we need to shut up the type checker
first. This should do it:

(499 as dynamic).doesNotExist();

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:23: void fallThrough() {
FallThrough errors should not exist at runtime in Dart 2.0. You can kill this
entire function and the code that references it.

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:37: 
Likewise, there is no AbstractClassInstantiationError in Dart 2.0. Delete this
fn.

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:51: return [1, 2].single;//# static:
compile-time error
As far as I can tell, this shouldn't have a static error. We do want to test
this case, but I think the previous code will be statically clean and throw a
runtime StateError like we want.

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:55: return 1 + "string";//# static:
compile-time error
I don't believe Dart 2.0 has TypeError. But it *does* have CastError, which this
test doesn't seem to cast, but should. How about changing this to:

void cast() {
  dynamic d = 1;
  d as String;
}

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:79: stackTrace is StackTrace, "$e
doesn't have a non-null stack trace");
I don't think this change is right. My reading is that this test is about
validating that Error itself does have a .stackTrace property.

[jcollins, 2017-08-02 21:03:26]

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/corelib...
File tests/corelib_2/corelib_2.status (right):

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/corelib...
tests/corelib_2/corelib_2.status:25: duration2_test: Skip # Static verification
only
On 2017/07/25 22:05:43, Bob Nystrom wrote:
> I think we can just delete this test. It's validating what happens if you pass
a
> string to new Duration(), but that's a type error now. I don't think we need
to
> keep it around for validating the static error since there's nothing
> particularly interesting about this particular static error.

Done.

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
File tests/corelib_2/error_stack_trace_test.dart (right):

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:10: 
On 2017/07/25 22:05:43, Bob Nystrom wrote:
> Oof, this test is complicated. I believe the intent here is to cause the
runtime
> to throw one of each of the various exception classes that extend Error. Then,
> for each of those, validate that it has a .stackTrace property that returns a
> non-null stack trace.
> 
> Some of these errors will disappear entirely in 2.0 because they are reported
> statically an no longer need to be thrown at runtime. For those, I think you
can
> delete the code in question. Others can still be thrown randomly, but you
might
> need some explicit "dynamic" to avoid hitting a static error first. The rest
> should be fine as-is.
> 
> I don't think you'll need to split out the static errors into a separate
> multitest.
> 
> In more detail...

Acknowledged.

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:12: (499).doesNotExist(); //#
static: compile-time error
On 2017/07/25 22:05:43, Bob Nystrom wrote:
> This is still a valid runtime error, but we need to shut up the type checker
> first. This should do it:
> 
> (499 as dynamic).doesNotExist();

So, in other words, in Dart 2.0 the only time a runtime error for non-existent
methods can happen is on dynamic declared types, and you think this test was
actually intended to catch just that?  If so, that works for me.  Done.

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:28: nested();//# static:
compile-time error
This case has become more interesting once we've peeled back the other ones. 
dartdevc is the only compiler that gives an error here; the analyzer gives
static warnings.  It suggests to me that the case is interesting in some way if
for no other reason than the different behavior, so I'm inclined not to drop it,
but will defer to your judgment on that.

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:37: 
On 2017/07/25 22:05:43, Bob Nystrom wrote:
> Likewise, there is no AbstractClassInstantiationError in Dart 2.0. Delete this
> fn.

done

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:51: return [1, 2].single;//# static:
compile-time error
On 2017/07/25 22:05:43, Bob Nystrom wrote:
> As far as I can tell, this shouldn't have a static error. We do want to test
> this case, but I think the previous code will be statically clean and throw a
> runtime StateError like we want.

This has a static error because the code returns an integer instead of a void. 
Altered based on your reading of what the purpose of this test is to return a
dynamic, thus creating a runtime error.

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:55: return 1 + "string";//# static:
compile-time error
On 2017/07/25 22:05:43, Bob Nystrom wrote:
> I don't believe Dart 2.0 has TypeError. But it *does* have CastError, which
this
> test doesn't seem to cast, but should. How about changing this to:
> 
> void cast() {
>   dynamic d = 1;
>   d as String;
> }

Done.

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:79: stackTrace is StackTrace, "$e
doesn't have a non-null stack trace");
On 2017/07/25 22:05:43, Bob Nystrom wrote:
> I don't think this change is right. My reading is that this test is about
> validating that Error itself does have a .stackTrace property.

I had asked around about this and the understanding I got was that the use of
the stackTrace property was obsolete.    Combine that with the fact that
dartdevc didn't have it in one case, I had inferred, perhaps incorrectly, that
Dart 2.0 no longer cares about that property.  Instead, extracted that case into
a multitest and will file a bug assuming that this is right.

[Bob Nystrom, 2017-08-02 21:12:36]

LGTM!

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
File tests/corelib_2/error_stack_trace_test.dart (right):

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:12: (499).doesNotExist(); //#
static: compile-time error
On 2017/08/02 21:03:26, jcollins wrote:
> On 2017/07/25 22:05:43, Bob Nystrom wrote:
> > This is still a valid runtime error, but we need to shut up the type checker
> > first. This should do it:
> > 
> > (499 as dynamic).doesNotExist();
> 
> So, in other words, in Dart 2.0 the only time a runtime error for non-existent
> methods can happen is on dynamic declared types,

Exactly right.

> and you think this test was
> actually intended to catch just that?

Yup!

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:28: nested();//# static:
compile-time error
On 2017/08/02 21:03:26, jcollins wrote:
> This case has become more interesting once we've peeled back the other ones. 
> dartdevc is the only compiler that gives an error here; the analyzer gives
> static warnings.

In strong mode, that static warning should get promoted to a static error. Once
strong mode is the only mode, there won't be a way to see this as a warning any
more.

> It suggests to me that the case is interesting in some way if
> for no other reason than the different behavior, so I'm inclined not to drop
it,
> but will defer to your judgment on that.

We probably have other cases where the existing behavior diverges for features
that are no longer supported by Dart 2.0, but that doesn't mean we need to keep
the test around. Part of the reason for going to Dart 2.0 is to end up with a
simpler system with less surface area to worry about, so deleting those tests is
a good thing — it means there's less opportunity for divergence in the first
place. :)

You can remove this code. As I understand it, FallThroughError literally won't
exist as a type in the Dart 2.0 core library, just like
AbstractClassInstantiationError.

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:51: return [1, 2].single;//# static:
compile-time error
On 2017/08/02 21:03:26, jcollins wrote:
> On 2017/07/25 22:05:43, Bob Nystrom wrote:
> > As far as I can tell, this shouldn't have a static error. We do want to test
> > this case, but I think the previous code will be statically clean and throw
a
> > runtime StateError like we want.
> 
> This has a static error because the code returns an integer instead of a void.


Ha, oh, right.

> Altered based on your reading of what the purpose of this test is to return a
> dynamic, thus creating a runtime error.

For this function, it just needs to *throw* a StateError. Calling single on a
list with more than one item will do that. I'd just remove the return:

void state() {
  [1, 2].single;
}

(If you want to be more minimal, [].first also throws a StateError.)

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:79: stackTrace is StackTrace, "$e
doesn't have a non-null stack trace");
On 2017/08/02 21:03:26, jcollins wrote:
> On 2017/07/25 22:05:43, Bob Nystrom wrote:
> > I don't think this change is right. My reading is that this test is about
> > validating that Error itself does have a .stackTrace property.
> 
> I had asked around about this and the understanding I got was that the use of
> the stackTrace property was obsolete.    Combine that with the fact that
> dartdevc didn't have it in one case, I had inferred, perhaps incorrectly, that
> Dart 2.0 no longer cares about that property.  Instead, extracted that case
into
> a multitest and will file a bug assuming that this is right.

Oh, interesting. I didn't realize there were any API changes to Error itself. It
might be good to tease those apart from the test migration, but I'll defer to
you on that.

[jcollins, 2017-08-03 15:54:18]

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
File tests/corelib_2/error_stack_trace_test.dart (right):

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:51: return [1, 2].single;//# static:
compile-time error
On 2017/08/02 21:12:36, Bob Nystrom wrote:
> On 2017/08/02 21:03:26, jcollins wrote:
> > On 2017/07/25 22:05:43, Bob Nystrom wrote:
> > > As far as I can tell, this shouldn't have a static error. We do want to
test
> > > this case, but I think the previous code will be statically clean and
throw
> a
> > > runtime StateError like we want.
> > 
> > This has a static error because the code returns an integer instead of a
void.
> 
> 
> Ha, oh, right.
> 
> > Altered based on your reading of what the purpose of this test is to return
a
> > dynamic, thus creating a runtime error.
> 
> For this function, it just needs to *throw* a StateError. Calling single on a
> list with more than one item will do that. I'd just remove the return:
> 
> void state() {
>   [1, 2].single;
> }
> 
> (If you want to be more minimal, [].first also throws a StateError.)

Making this more minimal makes sense, done.

https://codereview.chromium.org/2987793002/diff/20001/tests/corelib_2/error_s...
tests/corelib_2/error_stack_trace_test.dart:79: stackTrace is StackTrace, "$e
doesn't have a non-null stack trace");
On 2017/08/02 21:12:36, Bob Nystrom wrote:
> On 2017/08/02 21:03:26, jcollins wrote:
> > On 2017/07/25 22:05:43, Bob Nystrom wrote:
> > > I don't think this change is right. My reading is that this test is about
> > > validating that Error itself does have a .stackTrace property.
> > 
> > I had asked around about this and the understanding I got was that the use
of
> > the stackTrace property was obsolete.    Combine that with the fact that
> > dartdevc didn't have it in one case, I had inferred, perhaps incorrectly,
that
> > Dart 2.0 no longer cares about that property.  Instead, extracted that case
> into
> > a multitest and will file a bug assuming that this is right.
> 
> Oh, interesting. I didn't realize there were any API changes to Error itself.
It
> might be good to tease those apart from the test migration, but I'll defer to
> you on that.

Since I've already done the work of figuring out what case this is, might as
well leave a bread crumb or two.

[jcollins, 2017-08-03 15:55:36]

Description was changed from

==========
Migrate test block 7 to Dart 2.0.

duration2_test.dart converted to a static-only test and skipped where
appropriate for non-strong.  error_stack_trace_test converted to a multitest and
error_stack_trace1_test modified to use more modern catch convention for stack
traces.
==========

to

==========
Migrate test block 7 to Dart 2.0.

duration2_test.dart converted to a static-only test and skipped where
appropriate for non-strong.  error_stack_trace_test converted to a multitest and
error_stack_trace1_test modified to use more modern catch convention for stack
traces.

R=bkonyi@google.com, rnystrom@google.com

Committed:
https://github.com/dart-lang/sdk/commit/28a856828b0ce3928e798a694424b2b377784f79
==========

[jcollins, 2017-08-03 15:55:37]

Committed patchset #4 (id:60001) manually as
28a856828b0ce3928e798a694424b2b377784f79 (presubmit successful).