From 0211df9cfc2f8c51931b7382f8e4af08a1417629 Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Wed, 2 Nov 2022 11:43:42 -0700 Subject: [PATCH] [flutter_tools] provide --timeout option to flutter drive (#114458) --- .../tasks/new_gallery__transition_perf.dart | 9 ++- dev/devicelab/lib/tasks/new_gallery.dart | 1 + dev/devicelab/lib/tasks/perf_tests.dart | 14 +++- .../flutter_tools/lib/src/commands/drive.dart | 77 +++++++++++++++---- .../commands.shard/hermetic/drive_test.dart | 68 ++++++++++++++++ 5 files changed, 152 insertions(+), 17 deletions(-) diff --git a/dev/devicelab/bin/tasks/new_gallery__transition_perf.dart b/dev/devicelab/bin/tasks/new_gallery__transition_perf.dart index ae07dc99ac..dfac78c5b7 100644 --- a/dev/devicelab/bin/tasks/new_gallery__transition_perf.dart +++ b/dev/devicelab/bin/tasks/new_gallery__transition_perf.dart @@ -17,7 +17,14 @@ Future main() async { final Directory galleryDir = Directory(path.join(galleryParentDir.path, 'gallery')); try { - await task(NewGalleryPerfTest(galleryDir).run); + await task( + NewGalleryPerfTest( + galleryDir, + // time out after 20 minutes allowing the tool to take a screenshot to debug + // https://github.com/flutter/flutter/issues/114025. + timeoutSeconds: 20 * 60, + ).run, + ); } finally { rmTree(galleryParentDir); } diff --git a/dev/devicelab/lib/tasks/new_gallery.dart b/dev/devicelab/lib/tasks/new_gallery.dart index 8f2680d725..e7695c8631 100644 --- a/dev/devicelab/lib/tasks/new_gallery.dart +++ b/dev/devicelab/lib/tasks/new_gallery.dart @@ -15,6 +15,7 @@ class NewGalleryPerfTest extends PerfTest { String timelineFileName = 'transitions', String dartDefine = '', bool enableImpeller = false, + super.timeoutSeconds, }) : super( galleryDir.path, 'test_driver/transitions_perf.dart', diff --git a/dev/devicelab/lib/tasks/perf_tests.dart b/dev/devicelab/lib/tasks/perf_tests.dart index 85bfcecaa9..f52f804485 100644 --- a/dev/devicelab/lib/tasks/perf_tests.dart +++ b/dev/devicelab/lib/tasks/perf_tests.dart @@ -914,6 +914,7 @@ class PerfTest { this.device, this.flutterDriveCallback, this.enableImpeller = false, + this.timeoutSeconds, }): _resultFilename = resultFilename; const PerfTest.e2e( @@ -929,6 +930,7 @@ class PerfTest { this.device, this.flutterDriveCallback, this.enableImpeller = false, + this.timeoutSeconds, }) : saveTraceFile = false, timelineFileName = null, _resultFilename = resultFilename; /// The directory where the app under test is defined. @@ -963,6 +965,9 @@ class PerfTest { /// Whether the perf test should enable Impeller. final bool enableImpeller; + /// Number of seconds to time out the test after, allowing debug callbacks to run. + final int? timeoutSeconds; + /// The keys of the values that need to be reported. /// /// If it's `null`, then report: @@ -1020,6 +1025,11 @@ class PerfTest { '-v', '--verbose-system-logs', '--profile', + if (timeoutSeconds != null) + ...[ + '--timeout', + timeoutSeconds.toString(), + ], if (needsFullTimeline) '--trace-startup', // Enables "endless" timeline event buffering. '-t', testTarget, @@ -1039,7 +1049,7 @@ class PerfTest { if (flutterDriveCallback != null) { flutterDriveCallback!(options); } else { - await flutter('drive', options:options); + await flutter('drive', options: options); } final Map data = json.decode( file('${_testOutputDirectory(testDirectory)}/$resultFilename.json').readAsStringSync(), @@ -1140,7 +1150,7 @@ class PerfTestWithSkSL extends PerfTest { ); @override - Future run() async { + Future run({int? timeoutSeconds}) async { return inDirectory(testDirectory, () async { // Some initializations _device = await devices.workingDevice; diff --git a/packages/flutter_tools/lib/src/commands/drive.dart b/packages/flutter_tools/lib/src/commands/drive.dart index e33f2aecbd..79c587bdbe 100644 --- a/packages/flutter_tools/lib/src/commands/drive.dart +++ b/packages/flutter_tools/lib/src/commands/drive.dart @@ -150,7 +150,12 @@ class DriveCommand extends RunCommandBase { 'Dart VM running The test script.') ..addOption('profile-memory', help: 'Launch devtools and profile application memory, writing ' 'The output data to the file path provided to this argument as JSON.', - valueHelp: 'profile_memory.json'); + valueHelp: 'profile_memory.json') + ..addOption('timeout', + help: 'Timeout the test after the given number of seconds. If the ' + '"--screenshot" option is provided, a screenshot will be taken ' + 'before exiting. Defaults to no timeout.', + valueHelp: '360'); } final Signals signals; @@ -173,6 +178,8 @@ class DriveCommand extends RunCommandBase { final FileSystem _fileSystem; final Logger _logger; final FileSystemUtils _fsUtils; + Timer? timeoutTimer; + Map? screenshotTokens; @override final String name = 'drive'; @@ -295,13 +302,17 @@ class DriveCommand extends RunCommandBase { androidEmulator: boolArgDeprecated('android-emulator'), profileMemory: stringArgDeprecated('profile-memory'), ); - // If the test is sent a signal, take a screenshot before exiting - final Map screenshotTokens = _registerScreenshotCallbacks((ProcessSignal signal) async { - _logger.printError('Caught $signal'); - await _takeScreenshot(device); - }); + + // If the test is sent a signal or times out, take a screenshot + _registerScreenshotCallbacks(device); + final int testResult = await testResultFuture; - _unregisterScreenshotCallbacks(screenshotTokens); + + if (timeoutTimer != null) { + timeoutTimer!.cancel(); + } + _unregisterScreenshotCallbacks(); + if (testResult != 0 && screenshot != null) { // Take a screenshot while the app is still running. await _takeScreenshot(device); @@ -331,21 +342,59 @@ class DriveCommand extends RunCommandBase { return FlutterCommandResult.success(); } - Map _registerScreenshotCallbacks(Function(ProcessSignal) callback) { + int? get _timeoutSeconds { + final String? timeoutString = stringArg('timeout'); + if (timeoutString == null) { + return null; + } + final int? timeoutSeconds = int.tryParse(timeoutString); + if (timeoutSeconds == null || timeoutSeconds <= 0) { + throwToolExit( + 'Invalid value "$timeoutString" provided to the option --timeout: ' + 'expected a positive integer representing seconds.', + ); + } + return timeoutSeconds; + } + + void _registerScreenshotCallbacks(Device device) { _logger.printTrace('Registering signal handlers...'); final Map tokens = {}; for (final ProcessSignal signal in signalsToHandle) { - tokens[signal] = signals.addHandler(signal, callback); + tokens[signal] = signals.addHandler( + signal, + (ProcessSignal signal) { + _unregisterScreenshotCallbacks(); + _logger.printError('Caught $signal'); + return _takeScreenshot(device); + }, + ); + } + screenshotTokens = tokens; + + final int? timeoutSeconds = _timeoutSeconds; + if (timeoutSeconds != null) { + timeoutTimer = Timer( + Duration(seconds: timeoutSeconds), + () { + _unregisterScreenshotCallbacks(); + _takeScreenshot(device); + throwToolExit('Timed out after $timeoutSeconds seconds'); + } + ); } - return tokens; } - void _unregisterScreenshotCallbacks(Map tokens) { - _logger.printTrace('Unregistering signal handlers...'); - for (final MapEntry entry in tokens.entries) { - signals.removeHandler(entry.key, entry.value); + void _unregisterScreenshotCallbacks() { + if (screenshotTokens != null) { + _logger.printTrace('Unregistering signal handlers...'); + for (final MapEntry entry in screenshotTokens!.entries) { + signals.removeHandler(entry.key, entry.value); + } } + timeoutTimer?.cancel(); } + String? _getTestFile() { if (argResults!['driver'] != null) { return stringArgDeprecated('driver'); diff --git a/packages/flutter_tools/test/commands.shard/hermetic/drive_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/drive_test.dart index 28be04895e..6e10ff510a 100644 --- a/packages/flutter_tools/test/commands.shard/hermetic/drive_test.dart +++ b/packages/flutter_tools/test/commands.shard/hermetic/drive_test.dart @@ -5,8 +5,10 @@ import 'dart:async'; import 'dart:io' as io; +import 'package:fake_async/fake_async.dart'; import 'package:file/memory.dart'; import 'package:flutter_tools/src/application_package.dart'; +import 'package:flutter_tools/src/base/async_guard.dart'; import 'package:flutter_tools/src/base/common.dart'; import 'package:flutter_tools/src/base/file_system.dart'; import 'package:flutter_tools/src/base/io.dart'; @@ -203,6 +205,72 @@ void main() { DeviceManager: () => fakeDeviceManager, }); + testUsingContext('drive --timeout takes screenshot and tool exits after timeout', () async { + final DriveCommand command = DriveCommand( + fileSystem: fileSystem, + logger: logger, + platform: platform, + signals: Signals.test(), + flutterDriverFactory: NeverEndingFlutterDriverFactory(() {}), + ); + + fileSystem.file('lib/main.dart').createSync(recursive: true); + fileSystem.file('test_driver/main_test.dart').createSync(recursive: true); + fileSystem.file('pubspec.yaml').createSync(); + fileSystem.directory('drive_screenshots').createSync(); + + final ScreenshotDevice screenshotDevice = ScreenshotDevice(); + fakeDeviceManager.devices = [screenshotDevice]; + + expect(screenshotDevice.screenshots, isEmpty); + bool caughtToolExit = false; + FakeAsync().run((FakeAsync time) { + // Because the tool exit will be thrown asynchronously by a [Timer], + // use [asyncGuard] to catch it + asyncGuard( + () => createTestCommandRunner(command).run( + [ + 'drive', + '--no-pub', + '-d', + screenshotDevice.id, + '--use-existing-app', + 'http://localhost:8181', + '--screenshot', + 'drive_screenshots', + '--timeout', + '300', // 5 minutes + ], + ), + onError: (Object error) { + expect(error, isA()); + expect( + (error as ToolExit).message, + contains('Timed out after 300 seconds'), + ); + caughtToolExit = true; + } + ); + time.elapse(const Duration(seconds: 299)); + expect(screenshotDevice.screenshots, isEmpty); + time.elapse(const Duration(seconds: 2)); + expect( + screenshotDevice.screenshots, + contains(isA().having( + (File file) => file.path, + 'path', + 'drive_screenshots/drive_01.png', + )), + ); + }); + expect(caughtToolExit, isTrue); + }, overrides: { + FileSystem: () => fileSystem, + ProcessManager: () => FakeProcessManager.any(), + Pub: () => FakePub(), + DeviceManager: () => fakeDeviceManager, + }); + testUsingContext('drive --screenshot takes screenshot if sent a registered signal', () async { final FakeProcessSignal signal = FakeProcessSignal(); final ProcessSignal signalUnderTest = ProcessSignal(signal);