From 817fa9701467feaa8ac7bb4d4446548b983fcaba Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Thu, 5 Jun 2025 12:10:14 -0400 Subject: [PATCH] Combine expression evaluation tests to reduce testing time (#169860) Each `flutter run` invocation is expensive (time- and memory-wise). Each invocation also launches its own Chrome instance. This PR is attempting to minimize the number of launches by combining tests that need the same `flutter run` configuration into a single test. That way, we are reusing a single instance to do many checks, instead of launching a new instance for each check. Currently, I did this for expression evaluation tests, which cut their runtime in half on my local machine. This PR aims to help with issues like https://github.com/flutter/flutter/issues/169574 and https://github.com/flutter/flutter/issues/169304. Closes https://github.com/flutter/flutter/issues/110879 --- .../test_data/hot_reload_errors_common.dart | 20 ++--- .../test_data/hot_reload_test_common.dart | 15 ++-- .../test_data/hot_reload_with_asset.dart | 9 +- .../hot_reload_with_asset_test_common.dart | 38 ++------ ...valuation_web_ddc_library_bundle_test.dart | 1 - .../web.shard/hot_reload_web_errors_test.dart | 10 +-- .../hot_reload_with_asset_web_test.dart | 1 - .../expression_evaluation_web_common.dart | 87 ++++--------------- .../hot_restart_web_test_common.dart | 36 +++----- 9 files changed, 55 insertions(+), 162 deletions(-) diff --git a/packages/flutter_tools/test/integration.shard/test_data/hot_reload_errors_common.dart b/packages/flutter_tools/test/integration.shard/test_data/hot_reload_errors_common.dart index 0c8b9386de6..e8687b2f7d4 100644 --- a/packages/flutter_tools/test/integration.shard/test_data/hot_reload_errors_common.dart +++ b/packages/flutter_tools/test/integration.shard/test_data/hot_reload_errors_common.dart @@ -14,7 +14,6 @@ import 'hot_reload_const_project.dart'; void testAll({ bool chrome = false, List additionalCommandArgs = const [], - String constClassFieldRemovalErrorMessage = 'Try performing a hot restart instead.', Object? skip = false, }) { group('chrome: $chrome' @@ -35,35 +34,28 @@ void testAll({ }); testWithoutContext( - 'hot reload displays a formatted error message when removing a field from a const class', + 'hot reload displays a formatted error message when removing a field from a const class, and hot restart succeeds', () async { await flutter.run( device: chrome ? GoogleChromeDevice.kChromeDeviceId : FlutterTesterDevices.kTesterDeviceId, additionalCommandArgs: additionalCommandArgs, ); - project.removeFieldFromConstClass(); - expect( + project.removeFieldFromConstClass(); + await expectLater( flutter.hotReload(), throwsA( isA().having( (Exception e) => e.toString(), 'message', - contains(constClassFieldRemovalErrorMessage), + contains('Try performing a hot restart instead.'), ), ), ); + + await expectLater(flutter.hotRestart(), completes); }, ); - - testWithoutContext('hot restart succeeds when removing a field from a const class', () async { - await flutter.run( - device: chrome ? GoogleChromeDevice.kChromeDeviceId : FlutterTesterDevices.kTesterDeviceId, - additionalCommandArgs: additionalCommandArgs, - ); - project.removeFieldFromConstClass(); - await flutter.hotRestart(); - }); }); } diff --git a/packages/flutter_tools/test/integration.shard/test_data/hot_reload_test_common.dart b/packages/flutter_tools/test/integration.shard/test_data/hot_reload_test_common.dart index f6cf2435b0f..fbf0ac2f921 100644 --- a/packages/flutter_tools/test/integration.shard/test_data/hot_reload_test_common.dart +++ b/packages/flutter_tools/test/integration.shard/test_data/hot_reload_test_common.dart @@ -32,21 +32,18 @@ void testAll({bool chrome = false, List additionalCommandArgs = const subscription = flutter.stdout diff --git a/packages/flutter_tools/test/integration.shard/test_data/hot_reload_with_asset.dart b/packages/flutter_tools/test/integration.shard/test_data/hot_reload_with_asset.dart index 95731f80179..93c9edc945d 100644 --- a/packages/flutter_tools/test/integration.shard/test_data/hot_reload_with_asset.dart +++ b/packages/flutter_tools/test/integration.shard/test_data/hot_reload_with_asset.dart @@ -39,7 +39,7 @@ class MyApp extends StatelessWidget { Widget build(BuildContext context) { rootBundle.evict('pubspec.yaml'); rootBundle.load('pubspec.yaml').then((_) { - print('LOADED DATA'); + print('LOADED DATA'); // HOT RELOAD PRINT }, onError: (dynamic error, StackTrace stackTrace) { print('FAILED TO LOAD'); }); @@ -48,8 +48,11 @@ class MyApp extends StatelessWidget { } '''; - void uncommentHotReloadPrint() { - final String newMainContents = main.replaceAll('LOADED DATA', 'SECOND DATA'); + void replaceHotReloadPrint(String newPrint) { + final String newMainContents = main.replaceAll( + RegExp(r"print\('.*'\); // HOT RELOAD PRINT"), + "print('$newPrint'); // HOT RELOAD PRINT", + ); writeFile( fileSystem.path.join(dir.path, 'lib', 'main.dart'), newMainContents, diff --git a/packages/flutter_tools/test/integration.shard/test_data/hot_reload_with_asset_test_common.dart b/packages/flutter_tools/test/integration.shard/test_data/hot_reload_with_asset_test_common.dart index 7525565c3d2..7f3f5fc28d5 100644 --- a/packages/flutter_tools/test/integration.shard/test_data/hot_reload_with_asset_test_common.dart +++ b/packages/flutter_tools/test/integration.shard/test_data/hot_reload_with_asset_test_common.dart @@ -31,9 +31,10 @@ void testAll({bool chrome = false, List additionalCommandArgs = const onFirstLoad = Completer(); final Completer onSecondLoad = Completer(); + final Completer onThirdLoad = Completer(); flutter.stdout.listen((String line) { // If the asset fails to load, this message will be printed instead. @@ -48,6 +49,9 @@ void testAll({bool chrome = false, List additionalCommandArgs = const additionalCommandArgs = const onFirstLoad = Completer(); - final Completer onSecondLoad = Completer(); - - flutter.stdout.listen((String line) { - // If the asset fails to load, this message will be printed instead. - // this indicates that the devFS was not able to locate the asset - // after the hot reload. - if (line.contains('FAILED TO LOAD')) { - fail('Did not load asset: $line'); - } - if (line.contains('LOADED DATA')) { - onFirstLoad.complete(); - } - if (line.contains('SECOND DATA')) { - onSecondLoad.complete(); - } - }); - flutter.stdout.listen(printOnFailure); - await flutter.run( - device: chrome ? GoogleChromeDevice.kChromeDeviceId : FlutterTesterDevices.kTesterDeviceId, - additionalCommandArgs: additionalCommandArgs, - ); - await onFirstLoad.future; - - project.uncommentHotReloadPrint(); + project.replaceHotReloadPrint('THIRD DATA'); await flutter.hotRestart(); - await onSecondLoad.future; + await onThirdLoad.future; }); }); } diff --git a/packages/flutter_tools/test/web.shard/expression_evaluation_web_ddc_library_bundle_test.dart b/packages/flutter_tools/test/web.shard/expression_evaluation_web_ddc_library_bundle_test.dart index f95ddc338ec..7062b6e4479 100644 --- a/packages/flutter_tools/test/web.shard/expression_evaluation_web_ddc_library_bundle_test.dart +++ b/packages/flutter_tools/test/web.shard/expression_evaluation_web_ddc_library_bundle_test.dart @@ -2,7 +2,6 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -@TestOn('linux') // https://github.com/flutter/flutter/issues/169304 @Tags(['flutter-test-driver']) library; diff --git a/packages/flutter_tools/test/web.shard/hot_reload_web_errors_test.dart b/packages/flutter_tools/test/web.shard/hot_reload_web_errors_test.dart index de811a8457b..f71a333cab0 100644 --- a/packages/flutter_tools/test/web.shard/hot_reload_web_errors_test.dart +++ b/packages/flutter_tools/test/web.shard/hot_reload_web_errors_test.dart @@ -2,7 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -@TestOn('linux') // https://github.com/flutter/flutter/issues/169304 +@TestOn('linux') // https://github.com/flutter/flutter/issues/170062 @Tags(['flutter-test-driver']) library; @@ -10,11 +10,5 @@ import '../integration.shard/test_data/hot_reload_errors_common.dart'; import '../src/common.dart'; void main() { - testAll( - chrome: true, - additionalCommandArgs: ['--web-experimental-hot-reload'], - // TODO(srujzs): Remove this custom message once we have the delta inspector emitting the same - // string as the VM. - constClassFieldRemovalErrorMessage: 'Const class cannot remove fields', - ); + testAll(chrome: true, additionalCommandArgs: ['--web-experimental-hot-reload']); } diff --git a/packages/flutter_tools/test/web.shard/hot_reload_with_asset_web_test.dart b/packages/flutter_tools/test/web.shard/hot_reload_with_asset_web_test.dart index 6621653ac68..45551c06a08 100644 --- a/packages/flutter_tools/test/web.shard/hot_reload_with_asset_web_test.dart +++ b/packages/flutter_tools/test/web.shard/hot_reload_with_asset_web_test.dart @@ -2,7 +2,6 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -@TestOn('linux') // https://github.com/flutter/flutter/issues/169304 @Tags(['flutter-test-driver']) library; diff --git a/packages/flutter_tools/test/web.shard/test_data/expression_evaluation_web_common.dart b/packages/flutter_tools/test/web.shard/test_data/expression_evaluation_web_common.dart index c3eb5114c35..0491f5d8316 100644 --- a/packages/flutter_tools/test/web.shard/test_data/expression_evaluation_web_common.dart +++ b/packages/flutter_tools/test/web.shard/test_data/expression_evaluation_web_common.dart @@ -70,60 +70,19 @@ Future testAll({required bool useDDCLibraryBundleFormat}) async { await failToEvaluateExpression(flutter); }); - testWithoutContext('shows no native javascript objects in static scope', () async { + testWithoutContext('can evaluate expressions in library, top level and build method', () async { await start(expressionEvaluation: true); + + await evaluateTrivialExpressionsInLibrary(flutter); + await evaluateComplexExpressionsInLibrary(flutter); + await evaluateWebLibraryBooleanFromEnvironmentInLibrary(flutter); + await breakInTopLevelFunction(flutter); await checkStaticScope(flutter); - }); - - testWithoutContext('can handle compilation errors', () async { - await start(expressionEvaluation: true); - await breakInTopLevelFunction(flutter); await evaluateErrorExpressions(flutter); - }); - - testWithoutContext('can evaluate trivial expressions in top level function', () async { - await start(expressionEvaluation: true); - await breakInTopLevelFunction(flutter); await evaluateTrivialExpressions(flutter); - }); - - testWithoutContext('can evaluate trivial expressions in build method', () async { - await start(expressionEvaluation: true); - await breakInBuildMethod(flutter); - await evaluateTrivialExpressions(flutter); - }); - - testWithoutContext('can evaluate complex expressions in top level function', () async { - await start(expressionEvaluation: true); - await breakInTopLevelFunction(flutter); await evaluateComplexExpressions(flutter); - }); - testWithoutContext('can evaluate complex expressions in build method', () async { - await start(expressionEvaluation: true); - await breakInBuildMethod(flutter); - await evaluateComplexExpressions(flutter); - }); - - testWithoutContext('can evaluate trivial expressions in library without pause', () async { - await start(expressionEvaluation: true); - await evaluateTrivialExpressionsInLibrary(flutter); - }); - - testWithoutContext('can evaluate complex expressions in library without pause', () async { - await start(expressionEvaluation: true); - await evaluateComplexExpressionsInLibrary(flutter); - }); - - testWithoutContext('evaluated expression includes web library environment defines', () async { - await start(expressionEvaluation: true); - await evaluateWebLibraryBooleanFromEnvironmentInLibrary(flutter); - }); - - testWithoutContext('evaluated expression includes correctly mapped stack trace', () async { - await start(expressionEvaluation: true); - await breakInTopLevelFunction(flutter); // Test that the call comes from some Dart getter called `current` (the // location of which will be compiler-specific) and that the lines and // file name of the current location is correct and reports a Dart path. @@ -140,6 +99,10 @@ Future testAll({required bool useDDCLibraryBundleFormat}) async { end = stackTrace.indexOf('package:test/main.dart 15:7', end); return end != -1; }); + + await breakInBuildMethod(flutter); + await evaluateTrivialExpressions(flutter); + await evaluateComplexExpressions(flutter); }); }); @@ -190,35 +153,17 @@ Future testAll({required bool useDDCLibraryBundleFormat}) async { await failToEvaluateExpression(flutter); }); - testWithoutContext('can evaluate trivial expressions in a test', () async { + testWithoutContext('can evaluate expressions in a test', () async { await startPaused(expressionEvaluation: true); + + await evaluateTrivialExpressionsInLibrary(flutter); + await evaluateComplexExpressionsInLibrary(flutter); + await evaluateWebLibraryBooleanFromEnvironmentInLibrary(flutter); + await breakInMethod(flutter); await evaluateTrivialExpressions(flutter); - }); - - testWithoutContext('can evaluate complex expressions in a test', () async { - await startPaused(expressionEvaluation: true); - await breakInMethod(flutter); await evaluateComplexExpressions(flutter); - }); - testWithoutContext('can evaluate trivial expressions in library without pause', () async { - await startPaused(expressionEvaluation: true); - await evaluateTrivialExpressionsInLibrary(flutter); - }); - - testWithoutContext('can evaluate complex expressions in library without pause', () async { - await startPaused(expressionEvaluation: true); - await evaluateComplexExpressionsInLibrary(flutter); - }); - testWithoutContext('evaluated expression includes web library environment defines', () async { - await startPaused(expressionEvaluation: true); - await evaluateWebLibraryBooleanFromEnvironmentInLibrary(flutter); - }); - - testWithoutContext('evaluated expression includes correctly mapped stack trace', () async { - await startPaused(expressionEvaluation: true); - await breakInMethod(flutter); await evaluateStackTraceCurrent(flutter, (String stackTrace) { final Iterable matches = stackTraceCurrentRegexp.allMatches(stackTrace); if (matches.length != 1) { diff --git a/packages/flutter_tools/test/web.shard/test_data/hot_restart_web_test_common.dart b/packages/flutter_tools/test/web.shard/test_data/hot_restart_web_test_common.dart index add4581c374..cbcc9c4449d 100644 --- a/packages/flutter_tools/test/web.shard/test_data/hot_restart_web_test_common.dart +++ b/packages/flutter_tools/test/web.shard/test_data/hot_restart_web_test_common.dart @@ -84,31 +84,9 @@ Future _testProject( tryToDelete(tempDir); }); - testWithoutContext('$testName: hot restart works without error', () async { - flutter.stdout.listen(printOnFailure); - await flutter.run( - device: GoogleChromeDevice.kChromeDeviceId, - additionalCommandArgs: [ - '--verbose', - if (useDDCLibraryBundleFormat) - '--web-experimental-hot-reload' - else - '--no-web-experimental-hot-reload', - ], - ); - await flutter.hotRestart(); - }); - testWithoutContext( - '$testName: newly added code executes during hot restart - canvaskit', + '$testName: hot restart works without error and newly added code executes', () async { - final Completer completer = Completer(); - final StreamSubscription subscription = flutter.stdout.listen((String line) { - printOnFailure(line); - if (line.contains('(((((RELOAD WORKED)))))')) { - completer.complete(); - } - }); await flutter.run( device: GoogleChromeDevice.kChromeDeviceId, additionalCommandArgs: [ @@ -119,6 +97,16 @@ Future _testProject( '--no-web-experimental-hot-reload', ], ); + // hot restart works without error + await flutter.hotRestart(); + + final Completer completer = Completer(); + final StreamSubscription subscription = flutter.stdout.listen((String line) { + printOnFailure(line); + if (line.contains('(((((RELOAD WORKED)))))')) { + completer.complete(); + } + }); project.uncommentHotReloadPrint(); try { await flutter.hotRestart(); @@ -127,7 +115,5 @@ Future _testProject( await subscription.cancel(); } }, - // Skipped for https://github.com/flutter/flutter/issues/110879. - skip: true, ); }