Chaining the .. with the .then() makes this hard to immediately grok - maybe split it into distinct statements?

Done

The formatting style for this is:

FlutterCommandResult(
  this.exitCode, {
  this.analyticsParameters,
  this.exitTime,
}) {
  assert(exitCode != null);
}

Ok

final on all three of these.

Done

Nit: newline between field declarations.

Done

Maybe explain that FlutterCommand is what's doing the measurement so the reader knows where this automatic calculation will be made. Something like "If this is not provided, [FlutterCommand] will default to the command's completion time when calculating the command's running time"

Done

Is there a use case for commands providing their own entry time as well?

No. I think we should measure user perceivable time. That latency measurement should start as early as possible.

By starting this here, we'll record the time it takes to update the cache if needed (which is done in verifyThenRunCommand()), which will mess with the analytics results...

Ditto. I think that's fine as a metric to optimize for.

Is this used anymore? It looks like UsageTimer can be nuked.

Done. Love deleting code

Do you mean MockClock (or FakeClock given my comment below)?

Ah ya. I was gonna switch everything without breaking the existing tests but there were too many. Done. We can swap the others as needed.

It's such a simple interface, it may be worth writing a FakeClock and saving your tests the effort of mocking... It could have a setter for setting the current time, and it'd increment the value every time now() was called.

I think it's ok for the test to be verbose wrt the input/output near the assertions.

endTime? endTimeOverride?

Sure. Renamed to endTimeOverride

nit: I think flutter style would want single quotes here

ah thanks

There's a utility to print ms - getElapsedAsMilliseconds(Duration). Generally used for user facing output (non-trace output), but perhap useful here.

oh cool, thanks

you don't need the await here, since you're now returning the Future

I've heard that while the VM unwraps futures in these cases, it's bad style, but then I've also heard it's fiiiine. @abarth or @Hixie, is there a definitive answer?

I think semantically, we don't need to await here

Nit: for _ parameters, you can omit the type.

I'm confused as to what this is trying to do. Wouldn't scheduleMicrotask do the same thing cleanly? Or Future.sync?

oh cool, thanks

oops, our comments crossed paths midair.

I didn't really understand the references to Future in the Completer's dartdoc either. It seems to be written for the perspective of the Completer's reader rather than the Completer's supplier.

I'm trying to supply a Completer such that a piece of code runs synchronously when the owner of the Completer, a few layers deeper, calls complete() on it.

I'm not sure what "supply a completer" means. Normally a completer is private to the person who creates it, and you hand out its future.

@Hixie I had the same reaction, but it looks like it's an existing API in tools. I plan to remedy this in a separate PR by having the caller pass a callback instead of a completer.

cool neat. It doesn't actually seem heavily used in the codebase. A simpler callback would indeed be easier to parse.

If you're synced to tip-of-tree, this now needs to be await device.targetPlatform

thanks

Likewise, await device.isLocalEmulator

done

No need for the ? here

done

Similar formatting comment here:

  Duration duration, {
  String label,

done

We used to check for suppressAnalytics...

nice catch, thanks!

MockUsage and MockClock are seeded in the context for tests by default, so you don't need to instantiate and override them here. (same comment in the tests below)

so I ended up defaulting back to a real clock. I think it's more convenient as we move more and more stuff to Clock for tests that don't really care to keep working without null handling and not get an arbitrary value.

Nit: indent