From c7c8a6c4984a5b5bdbecd404417efee957713530 Mon Sep 17 00:00:00 2001 From: Zachary Anderson Date: Thu, 3 Oct 2019 11:08:42 -0700 Subject: [PATCH] [flutter_tools] Add more info to pub get failure event (#41652) --- .../flutter_tools/lib/src/base/process.dart | 12 +- .../lib/src/commands/packages.dart | 6 +- packages/flutter_tools/lib/src/dart/pub.dart | 35 ++++- .../lib/src/reporting/disabled_usage.dart | 2 +- .../lib/src/reporting/events.dart | 18 ++- .../lib/src/reporting/usage.dart | 9 +- .../commands.shard/hermetic/doctor_test.dart | 66 +++++++-- .../test/general.shard/dart/pub_get_test.dart | 126 +++++++++++------- .../general.shard/runner/runner_test.dart | 3 +- packages/flutter_tools/test/src/context.dart | 2 +- packages/flutter_tools/test/src/testbed.dart | 2 +- 11 files changed, 197 insertions(+), 84 deletions(-) diff --git a/packages/flutter_tools/lib/src/base/process.dart b/packages/flutter_tools/lib/src/base/process.dart index c012515d6a1..593bcd37ffa 100644 --- a/packages/flutter_tools/lib/src/base/process.dart +++ b/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(), ]); - await waitGroup(>[ - 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; } diff --git a/packages/flutter_tools/lib/src/commands/packages.dart b/packages/flutter_tools/lib/src/commands/packages.dart index fdfe70e1604..e77abcd7990 100644 --- a/packages/flutter_tools/lib/src/commands/packages.dart +++ b/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; } } diff --git a/packages/flutter_tools/lib/src/dart/pub.dart b/packages/flutter_tools/lib/src/dart/pub.dart index fd93aef7d00..9c2d74cedeb 100644 --- a/packages/flutter_tools/lib/src/dart/pub.dart +++ b/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 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 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 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.delayed(Duration(seconds: duration)); if (duration < 64) { @@ -179,6 +198,18 @@ Future 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); } diff --git a/packages/flutter_tools/lib/src/reporting/disabled_usage.dart b/packages/flutter_tools/lib/src/reporting/disabled_usage.dart index 2e326da67eb..c2c271b06f2 100644 --- a/packages/flutter_tools/lib/src/reporting/disabled_usage.dart +++ b/packages/flutter_tools/lib/src/reporting/disabled_usage.dart @@ -27,7 +27,7 @@ class DisabledUsage implements Usage { void sendCommand(String command, { Map parameters }) { } @override - void sendEvent(String category, String parameter, { Map parameters }) { } + void sendEvent(String category, String parameter, { String label, Map parameters }) { } @override void sendTiming(String category, String variableName, Duration duration, { String label }) { } diff --git a/packages/flutter_tools/lib/src/reporting/events.dart b/packages/flutter_tools/lib/src/reporting/events.dart index 77c320bf9bd..3a56458b0c9 100644 --- a/packages/flutter_tools/lib/src/reporting/events.dart +++ b/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. diff --git a/packages/flutter_tools/lib/src/reporting/usage.dart b/packages/flutter_tools/lib/src/reporting/usage.dart index 2618f3ce37d..4b30b0f2b70 100644 --- a/packages/flutter_tools/lib/src/reporting/usage.dart +++ b/packages/flutter_tools/lib/src/reporting/usage.dart @@ -124,6 +124,7 @@ abstract class Usage { void sendEvent( String category, String parameter, { + String label, Map parameters, }); @@ -262,6 +263,7 @@ class _DefaultUsage implements Usage { void sendEvent( String category, String parameter, { + String label, Map 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 diff --git a/packages/flutter_tools/test/commands.shard/hermetic/doctor_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/doctor_test.dart index 03014daaa90..179a1de4947 100644 --- a/packages/flutter_tools/test/commands.shard/hermetic/doctor_test.dart +++ b/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, ['installed', 'installed', 'installed'], ); }, overrides: { @@ -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, ['installed', 'installed'], ); expect( - verify(mockUsage.sendEvent('doctorResult.PartialValidatorWithHintsOnly', captureAny)).captured, + verify(mockUsage.sendEvent( + 'doctorResult.PartialValidatorWithHintsOnly', + captureAny, + label: anyNamed('label'), + )).captured, ['partial'], ); expect( - verify(mockUsage.sendEvent('doctorResult.PartialValidatorWithErrors', captureAny)).captured, + verify(mockUsage.sendEvent( + 'doctorResult.PartialValidatorWithErrors', + captureAny, + label: anyNamed('label'), + )).captured, ['partial'], ); }, overrides: { @@ -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, ['installed'], ); expect( - verify(mockUsage.sendEvent('doctorResult.MissingValidator', captureAny)).captured, + verify(mockUsage.sendEvent( + 'doctorResult.MissingValidator', + captureAny, + label: anyNamed('label'), + )).captured, ['missing'], ); expect( - verify(mockUsage.sendEvent('doctorResult.NotAvailableValidator', captureAny)).captured, + verify(mockUsage.sendEvent( + 'doctorResult.NotAvailableValidator', + captureAny, + label: anyNamed('label'), + )).captured, ['notAvailable'], ); expect( - verify(mockUsage.sendEvent('doctorResult.PartialValidatorWithHintsOnly', captureAny)).captured, + verify(mockUsage.sendEvent( + 'doctorResult.PartialValidatorWithHintsOnly', + captureAny, + label: anyNamed('label'), + )).captured, ['partial'], ); expect( - verify(mockUsage.sendEvent('doctorResult.PartialValidatorWithErrors', captureAny)).captured, + verify(mockUsage.sendEvent( + 'doctorResult.PartialValidatorWithErrors', + captureAny, + label: anyNamed('label'), + )).captured, ['partial'], ); }, overrides: { @@ -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, ['installed', 'installed', 'installed'], ); expect( - verify(mockUsage.sendEvent('doctorResult.MissingGroupedValidator', captureAny)).captured, + verify(mockUsage.sendEvent( + 'doctorResult.MissingGroupedValidator', + captureAny, + label: anyNamed('label'), + )).captured, ['missing'], ); }, overrides: { diff --git a/packages/flutter_tools/test/general.shard/dart/pub_get_test.dart b/packages/flutter_tools/test/general.shard/dart/pub_get_test.dart index 48093cafd8e..5cd68fdaf53 100644 --- a/packages/flutter_tools/test/general.shard/dart/pub_get_test.dart +++ b/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: {}, + environment: UnmodifiableMapView({}), ), }); @@ -115,7 +119,7 @@ void main() { ProcessManager: () => MockProcessManager(69), FileSystem: () => MockFileSystem(), Platform: () => FakePlatform( - environment: {}, + environment: UnmodifiableMapView({}), ), }); @@ -141,17 +145,78 @@ void main() { ProcessManager: () => MockProcessManager(69), FileSystem: () => MockFileSystem(), Platform: () => FakePlatform( - environment: {'PUB_CACHE': 'custom/pub-cache/path'}, + environment: UnmodifiableMapView({ + '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: { + ProcessManager: () => MockProcessManager(0), + FileSystem: () => MockFileSystem(), + Platform: () => FakePlatform( + environment: UnmodifiableMapView({ + '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: { + ProcessManager: () => MockProcessManager(1), + FileSystem: () => MockFileSystem(), + Platform: () => FakePlatform( + environment: UnmodifiableMapView({ + '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: { + ProcessManager: () => MockProcessManager( + 1, + stderr: 'version solving failed', + ), + FileSystem: () => MockFileSystem(), + Platform: () => FakePlatform( + environment: UnmodifiableMapView({ + 'PUB_CACHE': 'custom/pub-cache/path', + }), + ), + Usage: () => MockUsage(), + }); } typedef StartCallback = void Function(List 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.value(MockProcess(fakeExitCode)); + return Future.value(mocks.createMockProcess( + exitCode: fakeExitCode, + stderr: stderr, + )); } @override dynamic noSuchMethod(Invocation invocation) => null; } -class MockProcess implements Process { - MockProcess(this.fakeExitCode); - - final int fakeExitCode; - - @override - Stream> get stdout => MockStream>(); - - @override - Stream> get stderr => MockStream>(); - - @override - Future get exitCode => Future.value(fakeExitCode); - - @override - dynamic noSuchMethod(Invocation invocation) => null; -} - -class MockStream implements Stream { - @override - Stream transform(StreamTransformer streamTransformer) => MockStream(); - - @override - Stream where(bool test(T event)) => MockStream(); - - @override - StreamSubscription listen(void onData(T event), { Function onError, void onDone(), bool cancelOnError }) { - return MockStreamSubscription(); - } - - @override - dynamic noSuchMethod(Invocation invocation) => null; -} - -class MockStreamSubscription implements StreamSubscription { - @override - Future asFuture([ E futureValue ]) => Future.value(); - - @override - Future 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 {} diff --git a/packages/flutter_tools/test/general.shard/runner/runner_test.dart b/packages/flutter_tools/test/general.shard/runner/runner_test.dart index c903d574581..a3faaa881e5 100644 --- a/packages/flutter_tools/test/general.shard/runner/runner_test.dart +++ b/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 parameters, - }) => _impl.sendEvent(category, parameter, parameters: parameters); + }) => _impl.sendEvent(category, parameter, label: label, parameters: parameters); @override void sendTiming( diff --git a/packages/flutter_tools/test/src/context.dart b/packages/flutter_tools/test/src/context.dart index b4f7dead2c3..c3519b1a2f9 100644 --- a/packages/flutter_tools/test/src/context.dart +++ b/packages/flutter_tools/test/src/context.dart @@ -303,7 +303,7 @@ class FakeUsage implements Usage { void sendCommand(String command, { Map parameters }) { } @override - void sendEvent(String category, String parameter, { Map parameters }) { } + void sendEvent(String category, String parameter, { String label, Map parameters }) { } @override void sendTiming(String category, String variableName, Duration duration, { String label }) { } diff --git a/packages/flutter_tools/test/src/testbed.dart b/packages/flutter_tools/test/src/testbed.dart index 33bc93de8f7..b6186333314 100644 --- a/packages/flutter_tools/test/src/testbed.dart +++ b/packages/flutter_tools/test/src/testbed.dart @@ -162,7 +162,7 @@ class NoOpUsage implements Usage { void sendCommand(String command, {Map parameters}) {} @override - void sendEvent(String category, String parameter,{ Map parameters }) {} + void sendEvent(String category, String parameter, { String label, Map parameters }) {} @override void sendException(dynamic exception) {}