Make expectAsync be usable in strong mode

[mkustermann]

Basically every usage of expectAsync makes code non strong-mode compliant, because it takes in a precisely typed function type as a parameter but returns a plain Function:

$ cat test.dart
import 'package:test/test.dart';

typedef int IntegerFunction(int _);

int identity(int arg) => arg;

main() {
  test('test', () {
    IntegerFunction callback = expectAsync(identity);
    equals(callback(42), 42);
  });
}
$ dartanalyzer test.dart
Analyzing [test.dart]...
[warning] Unsound implicit cast from Function to (int) → int (test.dart, line 9, col 32)
1 warning found.

Since I'm not super familiar with strong-mode, I don't know, but there might be a way with generic methods to avoid this kind of issue.

[nex3]

I tried to do this as part of the initial strong-mode conversion, but it turns out not to be possible with the current type system. There are details in dart-lang/sdk#26420; we can properly type expectAsync() only once that's fixed.

For what it's worth, I personally don't consider test to be really usable in strong mode until this works.

[kevmoo]

We've agreed on the solution – ship some number of expectAsync0...expectAsyncN.

Should also discuss deprecating expectAsync.

[leafpetersen]

I believe @floitschG will drive this.

[kevmoo]

@floitschG Status here?

[floitschG]

Dan stated a week ago that @skybrian was working on a tool.
Brian, could you give an update? Thanks.

[skybrian]

I don't think we should wait for the tool to be ready, because it might be a while. I think we should go ahead and I'll use the tool if I see a way to do it. (I might be able to use pieces for a one-off migration tool just for this.)

Presumably we need to add expectAsync0..N, deprecate the old function, and have both of them available during a migration? Seems like we could get started?

[floitschG]

I agree. We should start by updating the package and add the new methods.

I'm not sure if we should deprecate the old method, since this would introduce a warning which is (inside Google) often treated as an error, thus leading to a breakage.

[keertip]

What is the status on this? This is blocking many tests for being strong clean.

[kevmoo]

I'm not sure if we should deprecate the old method, since this would introduce a warning which is (inside Google) often treated as an error, thus leading to a breakage.

Let's punt on marking it deprecated for this reason – although we should do it once we've rolled Google.

[dgrove]

@nex3 - can you take a look at this today?

On Fri, Nov 18, 2016 at 12:09 PM, Kevin Moore [email protected]
wrote:

I'm not sure if we should deprecate the old method, since this would
introduce a warning which is (inside Google) often treated as an error,
thus leading to a breakage.

Let's punt on marking it deprecated for this reason – although we should
do it once we've rolled Google.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#436 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACAsW5dBVNqV9m1ouU6cVVM7kREwzfgSks5q_gYHgaJpZM4IutQb
.

[nex3]

I'm not sure if we should deprecate the old method, since this would introduce a warning which is (inside Google) often treated as an error, thus leading to a breakage.

Avoiding useful warnings because they might cause churn sets a very bad precedent. Warnings exist as a category precisely so that we can communicate "you probably want to change this" without actually breaking code. If downstream teams want to keep their code warning-clean, that's great, but it has to be their responsibility to do so. If fatal warnings provide a strong reason for upstream maintainers not to add useful metadata, we should strongly discourage Google3 users from making warnings fatal.

@nex3 - can you take a look at this today?

I thought @floitschG was doing this?

I can do it if you want me to, but today is my 20% day, so I'd like to wait until Monday.

[dgrove]

On Fri, Nov 18, 2016 at 1:23 PM, Natalie Weizenbaum <
[email protected]> wrote:

I'm not sure if we should deprecate the old method, since this would
introduce a warning which is (inside Google) often treated as an error,
thus leading to a breakage.

Avoiding useful warnings because they might cause churn sets a very bad
precedent. Warnings exist as a category precisely so that we can
communicate "you probably want to change this" without actually breaking
code. If downstream teams want to keep their code warning-clean, that's
great, but it has to be their responsibility to do so. If fatal warnings
provides a reason for upstream maintainers not to add useful metadata,
we should strongly discourage Google3 users from making warnings fatal.

@nex3 https://github.com/nex3 - can you take a look at this today?

I thought @floitschG https://github.com/floitschG was doing this?

I can do it if you want me to, but today is my 20% day, so I'd like to
wait until Monday.

Works for me. thanks!

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#436 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACAsWwQHp4-wzmAjX8uovb9OmRYLK52Yks5q_hddgaJpZM4IutQb
.

[skybrian]

Deprecation warnings shouldn't break Google's build. (That used to be the case, but I'm pretty sure it's fixed now.)

[nex3]