[flutter_tools] Add more info to pub get failure event (#41652)

packages/flutter_tools/lib/src/base/process.dart

@@ -6,6 +6,7 @@ import 'dart:async';
 
 import '../convert.dart';
 import '../globals.dart';
+import 'common.dart';
 import 'context.dart';
 import 'file_system.dart';
 import 'io.dart';
@@ -470,10 +471,13 @@ class _DefaultProcessUtils implements ProcessUtils {
       stderrSubscription.asFuture<void>(),
     ]);
 
-    await waitGroup<void>(<Future<void>>[
-      stdoutSubscription.cancel(),
-      stderrSubscription.cancel(),
-    ]);
+    // The streams as futures have already completed, so waiting for the
+    // potentially async stream cancellation to complete likely has no benefit.
+    // Further, some Stream implementations commonly used in tests don't
+    // complete the Future returned here, which causes tests using
+    // mocks/FakeAsync to fail when these Futures are awaited.
+    unawaited(stdoutSubscription.cancel());
+    unawaited(stderrSubscription.cancel());
 
     return await process.exitCode;
   }

packages/flutter_tools/lib/src/commands/packages.dart

@@ -100,12 +100,10 @@ class PackagesGetCommand extends FlutterCommand {
         checkLastModified: false,
       );
       pubGetTimer.stop();
-      PubGetEvent(success: true).send();
-      flutterUsage.sendTiming('packages-pub-get', 'success', pubGetTimer.elapsed);
+      flutterUsage.sendTiming('pub', 'get', pubGetTimer.elapsed, label: 'success');
     } catch (_) {
       pubGetTimer.stop();
-      PubGetEvent(success: false).send();
-      flutterUsage.sendTiming('packages-pub-get', 'failure', pubGetTimer.elapsed);
+      flutterUsage.sendTiming('pub', 'get', pubGetTimer.elapsed, label: 'failure');
       rethrow;
     }
   }

packages/flutter_tools/lib/src/dart/pub.dart

@@ -15,6 +15,7 @@ import '../base/process.dart';
 import '../base/utils.dart';
 import '../cache.dart';
 import '../globals.dart';
+import '../reporting/reporting.dart';
 import '../runner/flutter_command.dart';
 import 'sdk.dart';
 
@@ -54,6 +55,10 @@ class PubContext {
 
   @override
   String toString() => 'PubContext: ${_values.join(':')}';
+
+  String toAnalyticsString()  {
+    return _values.map((String s) => s.replaceAll('_', '-')).toList().join('-');
+  }
 }
 
 bool _shouldRunPubGet({ File pubSpecYaml, File dotPackages }) {
@@ -128,7 +133,9 @@ Future<void> pubGet({
   }
 
   if (dotPackages.lastModifiedSync().isBefore(pubSpecYaml.lastModifiedSync())) {
-    throwToolExit('$directory: pub did not update .packages file (pubspec.yaml timestamp: ${pubSpecYaml.lastModifiedSync()}; .packages timestamp: ${dotPackages.lastModifiedSync()}).');
+    throwToolExit('$directory: pub did not update .packages file '
+                  '(pubspec.yaml timestamp: ${pubSpecYaml.lastModifiedSync()}; '
+                  '.packages timestamp: ${dotPackages.lastModifiedSync()}).');
   }
 }
 
@@ -155,6 +162,17 @@ Future<void> pub(
 }) async {
   showTraceForErrors ??= isRunningOnBot;
 
+  bool versionSolvingFailed = false;
+  String filterWrapper(String line) {
+    if (line.contains('version solving failed')) {
+      versionSolvingFailed = true;
+    }
+    if (filter == null) {
+      return line;
+    }
+    return filter(line);
+  }
+
   if (showTraceForErrors) {
     arguments.insert(0, '--trace');
   }
@@ -166,12 +184,13 @@ Future<void> pub(
     code = await processUtils.stream(
       _pubCommand(arguments),
       workingDirectory: directory,
-      mapFunction: filter,
+      mapFunction: filterWrapper,
       environment: _createPubEnvironment(context),
     );
     if (code != 69) { // UNAVAILABLE in https://github.com/dart-lang/pub/blob/master/lib/src/exit_codes.dart
       break;
     }
+    versionSolvingFailed = false;
     printStatus('$failureMessage ($code) -- attempting retry $attempts in $duration second${ duration == 1 ? "" : "s"}...');
     await Future<void>.delayed(Duration(seconds: duration));
     if (duration < 64) {
@@ -179,6 +198,18 @@ Future<void> pub(
     }
   }
   assert(code != null);
+
+  String result = 'success';
+  if (versionSolvingFailed) {
+    result = 'version-solving-failed';
+  } else if (code != 0) {
+    result = 'failure';
+  }
+  PubResultEvent(
+    context: context.toAnalyticsString(),
+    result: result,
+  ).send();
+
   if (code != 0) {
     throwToolExit('$failureMessage ($code)', exitCode: code);
   }

packages/flutter_tools/lib/src/reporting/disabled_usage.dart

@@ -27,7 +27,7 @@ class DisabledUsage implements Usage {
   void sendCommand(String command, { Map<String, String> parameters }) { }
 
   @override
-  void sendEvent(String category, String parameter, { Map<String, String> parameters }) { }
+  void sendEvent(String category, String parameter, { String label, Map<String, String> parameters }) { }
 
   @override
   void sendTiming(String category, String variableName, Duration duration, { String label }) { }

packages/flutter_tools/lib/src/reporting/events.dart

@@ -9,13 +9,16 @@ part of reporting;
 /// If sending values for custom dimensions is required, extend this class as
 /// below.
 class UsageEvent {
-  UsageEvent(this.category, this.parameter);
+  UsageEvent(this.category, this.parameter, {
+    this.label,
+  });
 
   final String category;
   final String parameter;
+  final String label;
 
   void send() {
-    flutterUsage.sendEvent(category, parameter);
+    flutterUsage.sendEvent(category, parameter, label: label);
   }
 }
 
@@ -101,7 +104,7 @@ class DoctorResultEvent extends UsageEvent {
   @override
   void send() {
     if (validator is! GroupedValidator) {
-      flutterUsage.sendEvent(category, parameter);
+      flutterUsage.sendEvent(category, parameter, label: label);
       return;
     }
     final GroupedValidator group = validator;
@@ -114,10 +117,11 @@ class DoctorResultEvent extends UsageEvent {
 }
 
 /// An event that reports success or failure of a pub get.
-class PubGetEvent extends UsageEvent {
-  PubGetEvent({
-    @required bool success,
-  }) : super('packages-pub-get', success ? 'success' : 'failure');
+class PubResultEvent extends UsageEvent {
+  PubResultEvent({
+    @required String context,
+    @required String result,
+  }) : super('pub', context, label: result);
 }
 
 /// An event that reports something about a build.

packages/flutter_tools/lib/src/reporting/usage.dart

@@ -124,6 +124,7 @@ abstract class Usage {
   void sendEvent(
     String category,
     String parameter, {
+    String label,
     Map<String, String> parameters,
   });
 
@@ -262,6 +263,7 @@ class _DefaultUsage implements Usage {
   void sendEvent(
     String category,
     String parameter, {
+    String label,
     Map<String, String> parameters,
   }) {
     if (suppressAnalytics) {
@@ -273,7 +275,12 @@ class _DefaultUsage implements Usage {
       cdKey(CustomDimensions.localTime): formatDateTime(systemClock.now()),
     };
 
-    _analytics.sendEvent(category, parameter, parameters: paramsWithLocalTime);
+    _analytics.sendEvent(
+      category,
+      parameter,
+      label: label,
+      parameters: paramsWithLocalTime,
+    );
   }
 
   @override

packages/flutter_tools/test/commands.shard/hermetic/doctor_test.dart

@@ -225,7 +225,11 @@ void main() {
       await doctor.diagnose(verbose: false);
 
       expect(
-        verify(mockUsage.sendEvent('doctorResult.PassingValidator', captureAny)).captured,
+        verify(mockUsage.sendEvent(
+          'doctorResult.PassingValidator',
+          captureAny,
+          label: anyNamed('label'),
+        )).captured,
         <dynamic>['installed', 'installed', 'installed'],
       );
     }, overrides: <Type, Generator>{
@@ -238,15 +242,27 @@ void main() {
       await FakePassingDoctor().diagnose(verbose: false);
 
       expect(
-        verify(mockUsage.sendEvent('doctorResult.PassingValidator', captureAny)).captured,
+        verify(mockUsage.sendEvent(
+          'doctorResult.PassingValidator',
+          captureAny,
+          label: anyNamed('label'),
+        )).captured,
         <dynamic>['installed', 'installed'],
       );
       expect(
-        verify(mockUsage.sendEvent('doctorResult.PartialValidatorWithHintsOnly', captureAny)).captured,
+        verify(mockUsage.sendEvent(
+          'doctorResult.PartialValidatorWithHintsOnly',
+          captureAny,
+          label: anyNamed('label'),
+        )).captured,
         <dynamic>['partial'],
       );
       expect(
-        verify(mockUsage.sendEvent('doctorResult.PartialValidatorWithErrors', captureAny)).captured,
+        verify(mockUsage.sendEvent(
+          'doctorResult.PartialValidatorWithErrors',
+          captureAny,
+          label: anyNamed('label'),
+        )).captured,
         <dynamic>['partial'],
       );
     }, overrides: <Type, Generator>{
@@ -258,23 +274,43 @@ void main() {
       await FakeDoctor().diagnose(verbose: false);
 
       expect(
-        verify(mockUsage.sendEvent('doctorResult.PassingValidator', captureAny)).captured,
+        verify(mockUsage.sendEvent(
+          'doctorResult.PassingValidator',
+          captureAny,
+          label: anyNamed('label'),
+        )).captured,
         <dynamic>['installed'],
       );
       expect(
-        verify(mockUsage.sendEvent('doctorResult.MissingValidator', captureAny)).captured,
+        verify(mockUsage.sendEvent(
+          'doctorResult.MissingValidator',
+          captureAny,
+          label: anyNamed('label'),
+        )).captured,
         <dynamic>['missing'],
       );
       expect(
-        verify(mockUsage.sendEvent('doctorResult.NotAvailableValidator', captureAny)).captured,
+        verify(mockUsage.sendEvent(
+          'doctorResult.NotAvailableValidator',
+          captureAny,
+          label: anyNamed('label'),
+        )).captured,
         <dynamic>['notAvailable'],
       );
       expect(
-        verify(mockUsage.sendEvent('doctorResult.PartialValidatorWithHintsOnly', captureAny)).captured,
+        verify(mockUsage.sendEvent(
+          'doctorResult.PartialValidatorWithHintsOnly',
+          captureAny,
+          label: anyNamed('label'),
+        )).captured,
         <dynamic>['partial'],
       );
       expect(
-        verify(mockUsage.sendEvent('doctorResult.PartialValidatorWithErrors', captureAny)).captured,
+        verify(mockUsage.sendEvent(
+          'doctorResult.PartialValidatorWithErrors',
+          captureAny,
+          label: anyNamed('label'),
+        )).captured,
         <dynamic>['partial'],
       );
     }, overrides: <Type, Generator>{
@@ -286,11 +322,19 @@ void main() {
       await FakeGroupedDoctor().diagnose(verbose: false);
 
       expect(
-        verify(mockUsage.sendEvent('doctorResult.PassingGroupedValidator', captureAny)).captured,
+        verify(mockUsage.sendEvent(
+          'doctorResult.PassingGroupedValidator',
+          captureAny,
+          label: anyNamed('label'),
+        )).captured,
         <dynamic>['installed', 'installed', 'installed'],
       );
       expect(
-        verify(mockUsage.sendEvent('doctorResult.MissingGroupedValidator', captureAny)).captured,
+        verify(mockUsage.sendEvent(
+          'doctorResult.MissingGroupedValidator',
+          captureAny,
+          label: anyNamed('label'),
+        )).captured,
         <dynamic>['missing'],
       );
     }, overrides: <Type, Generator>{

packages/flutter_tools/test/general.shard/dart/pub_get_test.dart

@@ -3,14 +3,17 @@
 // found in the LICENSE file.
 
 import 'dart:async';
+import 'dart:collection';
 
 import 'package:file/file.dart';
 import 'package:file/memory.dart';
 import 'package:flutter_tools/src/cache.dart';
+import 'package:flutter_tools/src/base/common.dart';
 import 'package:flutter_tools/src/base/context.dart';
 import 'package:flutter_tools/src/base/io.dart';
 import 'package:flutter_tools/src/base/platform.dart';
 import 'package:flutter_tools/src/dart/pub.dart';
+import 'package:flutter_tools/src/reporting/reporting.dart';
 
 import 'package:mockito/mockito.dart';
 import 'package:process/process.dart';
@@ -18,6 +21,7 @@ import 'package:quiver/testing/async.dart';
 
 import '../../src/common.dart';
 import '../../src/context.dart';
+import '../../src/mocks.dart' as mocks;
 
 void main() {
   setUpAll(() {
@@ -88,7 +92,7 @@ void main() {
     ProcessManager: () => MockProcessManager(69),
     FileSystem: () => MockFileSystem(),
     Platform: () => FakePlatform(
-      environment: <String, String>{},
+      environment: UnmodifiableMapView<String, String>(<String, String>{}),
     ),
   });
 
@@ -115,7 +119,7 @@ void main() {
     ProcessManager: () => MockProcessManager(69),
     FileSystem: () => MockFileSystem(),
     Platform: () => FakePlatform(
-      environment: <String, String>{},
+      environment: UnmodifiableMapView<String, String>(<String, String>{}),
     ),
   });
 
@@ -141,17 +145,78 @@ void main() {
     ProcessManager: () => MockProcessManager(69),
     FileSystem: () => MockFileSystem(),
     Platform: () => FakePlatform(
-      environment: <String, String>{'PUB_CACHE': 'custom/pub-cache/path'},
+      environment: UnmodifiableMapView<String, String>(<String, String>{
+        'PUB_CACHE': 'custom/pub-cache/path',
+      }),
     ),
   });
+
+  testUsingContext('analytics sent on success', () async {
+    MockDirectory.findCache = true;
+    await pubGet(context: PubContext.flutterTests, checkLastModified: false);
+    verify(flutterUsage.sendEvent('pub', 'flutter-tests', label: 'success')).called(1);
+  }, overrides: <Type, Generator>{
+    ProcessManager: () => MockProcessManager(0),
+    FileSystem: () => MockFileSystem(),
+    Platform: () => FakePlatform(
+      environment: UnmodifiableMapView<String, String>(<String, String>{
+        'PUB_CACHE': 'custom/pub-cache/path',
+      }),
+    ),
+    Usage: () => MockUsage(),
+  });
+
+  testUsingContext('analytics sent on failure', () async {
+    MockDirectory.findCache = true;
+    try {
+      await pubGet(context: PubContext.flutterTests, checkLastModified: false);
+    } on ToolExit {
+      // Ignore.
+    }
+    verify(flutterUsage.sendEvent('pub', 'flutter-tests', label: 'failure')).called(1);
+  }, overrides: <Type, Generator>{
+    ProcessManager: () => MockProcessManager(1),
+    FileSystem: () => MockFileSystem(),
+    Platform: () => FakePlatform(
+      environment: UnmodifiableMapView<String, String>(<String, String>{
+        'PUB_CACHE': 'custom/pub-cache/path',
+      }),
+    ),
+    Usage: () => MockUsage(),
+  });
+
+  testUsingContext('analytics sent on failed version solve', () async {
+    MockDirectory.findCache = true;
+    try {
+      await pubGet(context: PubContext.flutterTests, checkLastModified: false);
+    } on ToolExit {
+      // Ignore.
+    }
+    verify(flutterUsage.sendEvent('pub', 'flutter-tests', label: 'version-solving-failed')).called(1);
+  }, overrides: <Type, Generator>{
+    ProcessManager: () => MockProcessManager(
+      1,
+      stderr: 'version solving failed',
+    ),
+    FileSystem: () => MockFileSystem(),
+    Platform: () => FakePlatform(
+      environment: UnmodifiableMapView<String, String>(<String, String>{
+        'PUB_CACHE': 'custom/pub-cache/path',
+      }),
+    ),
+    Usage: () => MockUsage(),
+  });
 }
 
 typedef StartCallback = void Function(List<dynamic> command);
 
 class MockProcessManager implements ProcessManager {
-  MockProcessManager(this.fakeExitCode);
+  MockProcessManager(this.fakeExitCode, {
+    this.stderr = '',
+  });
 
   final int fakeExitCode;
+  final String stderr;
 
   String lastPubEnvironment;
   String lastPubCache;
@@ -167,59 +232,16 @@ class MockProcessManager implements ProcessManager {
   }) {
     lastPubEnvironment = environment['PUB_ENVIRONMENT'];
     lastPubCache = environment['PUB_CACHE'];
-    return Future<Process>.value(MockProcess(fakeExitCode));
-  }
-
-  @override
-  dynamic noSuchMethod(Invocation invocation) => null;
-}
-
-class MockProcess implements Process {
-  MockProcess(this.fakeExitCode);
-
-  final int fakeExitCode;
-
-  @override
-  Stream<List<int>> get stdout => MockStream<List<int>>();
-
-  @override
-  Stream<List<int>> get stderr => MockStream<List<int>>();
-
-  @override
-  Future<int> get exitCode => Future<int>.value(fakeExitCode);
-
-  @override
-  dynamic noSuchMethod(Invocation invocation) => null;
-}
-
-class MockStream<T> implements Stream<T> {
-  @override
-  Stream<S> transform<S>(StreamTransformer<T, S> streamTransformer) => MockStream<S>();
-
-  @override
-  Stream<T> where(bool test(T event)) => MockStream<T>();
-
-  @override
-  StreamSubscription<T> listen(void onData(T event), { Function onError, void onDone(), bool cancelOnError }) {
-    return MockStreamSubscription<T>();
+    return Future<Process>.value(mocks.createMockProcess(
+      exitCode: fakeExitCode,
+      stderr: stderr,
+    ));
   }
 
   @override
   dynamic noSuchMethod(Invocation invocation) => null;
 }
 
-class MockStreamSubscription<T> implements StreamSubscription<T> {
-  @override
-  Future<E> asFuture<E>([ E futureValue ]) => Future<E>.value();
-
-  @override
-  Future<void> cancel() async { }
-
-  @override
-  dynamic noSuchMethod(Invocation invocation) => null;
-}
-
-
 class MockFileSystem extends ForwardingFileSystem {
   MockFileSystem() : super(MemoryFileSystem());
 
@@ -266,3 +288,5 @@ class MockDirectory implements Directory {
 }
 
 class MockRandomAccessFile extends Mock implements RandomAccessFile {}
+
+class MockUsage extends Mock implements Usage {}

packages/flutter_tools/test/general.shard/runner/runner_test.dart

@@ -139,8 +139,9 @@ class CrashingUsage implements Usage {
   void sendEvent(
     String category,
     String parameter, {
+    String label,
     Map<String, String> parameters,
-  }) => _impl.sendEvent(category, parameter, parameters: parameters);
+  }) => _impl.sendEvent(category, parameter, label: label, parameters: parameters);
 
   @override
   void sendTiming(

packages/flutter_tools/test/src/context.dart

@@ -303,7 +303,7 @@ class FakeUsage implements Usage {
   void sendCommand(String command, { Map<String, String> parameters }) { }
 
   @override
-  void sendEvent(String category, String parameter, { Map<String, String> parameters }) { }
+  void sendEvent(String category, String parameter, { String label, Map<String, String> parameters }) { }
 
   @override
   void sendTiming(String category, String variableName, Duration duration, { String label }) { }

packages/flutter_tools/test/src/testbed.dart

@@ -162,7 +162,7 @@ class NoOpUsage implements Usage {
   void sendCommand(String command, {Map<String, String> parameters}) {}
 
   @override
-  void sendEvent(String category, String parameter,{ Map<String, String> parameters }) {}
+  void sendEvent(String category, String parameter, { String label, Map<String, String> parameters }) {}
 
   @override
   void sendException(dynamic exception) {}