From 5d013c73baa70a8b3e1c541cb63e4c22654aa3cc Mon Sep 17 00:00:00 2001 From: "auto-submit[bot]" <98614782+auto-submit[bot]@users.noreply.github.com> Date: Wed, 28 May 2025 12:35:47 +0000 Subject: [PATCH] Reverts "Initialize `default-flavor` in `FlutterCommand`, adds integration test. (#169298)" (#169581) Reverts: flutter/flutter#169298 Initiated by: matanlurey Reason for reverting: Broke mac/iOS integration tests. Original PR Author: matanlurey Reviewed By: {vashworth, bkonyi} This change reverts the following previous change: Closes https://github.com/flutter/flutter/issues/169160. Closes https://github.com/flutter/flutter/issues/165803. Co-authored-by: auto-submit[bot] --- .../lib/src/build_system/targets/common.dart | 16 ++- .../lib/src/runner/flutter_command.dart | 12 -- .../build_system/targets/common_test.dart | 109 ++++++++---------- .../runner/flutter_command_test.dart | 92 --------------- .../default_flavor_test.dart | 74 ------------ .../test/integration.shard/test_driver.dart | 5 +- 6 files changed, 64 insertions(+), 244 deletions(-) delete mode 100644 packages/flutter_tools/test/integration.shard/default_flavor_test.dart diff --git a/packages/flutter_tools/lib/src/build_system/targets/common.dart b/packages/flutter_tools/lib/src/build_system/targets/common.dart index 61583210e47..735c380b10f 100644 --- a/packages/flutter_tools/lib/src/build_system/targets/common.dart +++ b/packages/flutter_tools/lib/src/build_system/targets/common.dart @@ -6,14 +6,16 @@ import 'package:package_config/package_config.dart'; import '../../artifacts.dart'; import '../../base/build.dart'; +import '../../base/common.dart'; import '../../base/file_system.dart'; import '../../base/io.dart'; import '../../build_info.dart'; import '../../compile.dart'; import '../../dart/package_map.dart'; import '../../devfs.dart'; -import '../../globals.dart' as globals show xcode; +import '../../globals.dart' as globals show platform, xcode; import '../../project.dart'; +import '../../runner/flutter_command.dart'; import '../build_system.dart'; import '../depfile.dart'; import '../exceptions.dart'; @@ -308,10 +310,16 @@ class KernelSnapshot extends Target { if (flavor == null) { return; } - if (!dartDefines.any((String element) => element.startsWith(kAppFlavor))) { - // If the flavor is not already in the dart defines, add it. - dartDefines.add('$kAppFlavor=$flavor'); + if (globals.platform.environment[kAppFlavor] != null) { + throwToolExit('$kAppFlavor is used by the framework and cannot be set in the environment.'); } + if (dartDefines.any((String define) => define.startsWith(kAppFlavor))) { + throwToolExit( + '$kAppFlavor is used by the framework and cannot be ' + 'set using --${FlutterOptions.kDartDefinesOption} or --${FlutterOptions.kDartDefineFromFileOption}', + ); + } + dartDefines.add('$kAppFlavor=$flavor'); } } diff --git a/packages/flutter_tools/lib/src/runner/flutter_command.dart b/packages/flutter_tools/lib/src/runner/flutter_command.dart index 38ebd9ceb12..00f754f8591 100644 --- a/packages/flutter_tools/lib/src/runner/flutter_command.dart +++ b/packages/flutter_tools/lib/src/runner/flutter_command.dart @@ -1411,18 +1411,6 @@ abstract class FlutterCommand extends Command { final String? cliFlavor = argParser.options.containsKey('flavor') ? stringArg('flavor') : null; final String? flavor = cliFlavor ?? defaultFlavor; - if (globals.platform.environment[kAppFlavor] != null) { - throwToolExit('$kAppFlavor is used by the framework and cannot be set in the environment.'); - } - if (dartDefines.any((String define) => define.startsWith(kAppFlavor))) { - throwToolExit( - '$kAppFlavor is used by the framework and cannot be ' - 'set using --${FlutterOptions.kDartDefinesOption} or --${FlutterOptions.kDartDefineFromFileOption}', - ); - } - if (flavor != null) { - dartDefines.add('$kAppFlavor=$flavor'); - } _addFlutterVersionToDartDefines(globals.flutterVersion, dartDefines); return BuildInfo( diff --git a/packages/flutter_tools/test/general.shard/build_system/targets/common_test.dart b/packages/flutter_tools/test/general.shard/build_system/targets/common_test.dart index 65008211cc5..2e2ad77efe7 100644 --- a/packages/flutter_tools/test/general.shard/build_system/targets/common_test.dart +++ b/packages/flutter_tools/test/general.shard/build_system/targets/common_test.dart @@ -13,7 +13,6 @@ import 'package:flutter_tools/src/build_system/exceptions.dart'; import 'package:flutter_tools/src/build_system/targets/common.dart'; import 'package:flutter_tools/src/build_system/targets/ios.dart'; import 'package:flutter_tools/src/compile.dart'; -import 'package:flutter_tools/src/convert.dart'; import 'package:flutter_tools/src/ios/xcodeproj.dart'; import 'package:test/fake.dart'; @@ -440,6 +439,57 @@ void main() { }, ); + testUsingContext( + "tool exits when $kAppFlavor is already set in user's environment", + () async { + fileSystem.file('.dart_tool/package_config.json') + ..createSync(recursive: true) + ..writeAsStringSync('{"configVersion": 2, "packages":[]}'); + final Future buildResult = const KernelSnapshot().build( + androidEnvironment + ..defines[kTargetPlatform] = getNameForTargetPlatform(TargetPlatform.android) + ..defines[kBuildMode] = BuildMode.debug.cliName + ..defines[kFlavor] = 'strawberry' + ..defines[kTrackWidgetCreation] = 'false', + ); + + expect( + buildResult, + throwsToolExit( + message: '$kAppFlavor is used by the framework and cannot be set in the environment.', + ), + ); + }, + overrides: { + Platform: () => FakePlatform(environment: {kAppFlavor: 'I was already set'}), + }, + ); + + testUsingContext( + 'tool exits when $kAppFlavor is set in --dart-define or --dart-define-from-file', + () async { + fileSystem.file('.dart_tool/package_config.json') + ..createSync(recursive: true) + ..writeAsStringSync('{"configVersion": 2, "packages":[]}'); + final Future buildResult = const KernelSnapshot().build( + androidEnvironment + ..defines[kTargetPlatform] = getNameForTargetPlatform(TargetPlatform.android) + ..defines[kBuildMode] = BuildMode.debug.cliName + ..defines[kFlavor] = 'strawberry' + ..defines[kDartDefines] = encodeDartDefines([kAppFlavor, 'strawberry']) + ..defines[kTrackWidgetCreation] = 'false', + ); + + expect( + buildResult, + throwsToolExit( + message: + '$kAppFlavor is used by the framework and cannot be set using --dart-define or --dart-define-from-file', + ), + ); + }, + ); + testUsingContext( 'KernelSnapshot sets flavor in dartDefines from Xcode build configuration if ios app', () async { @@ -555,63 +605,6 @@ void main() { }, ); - testUsingContext( - 'KernelSnapshot does not add kAppFlavor twice to Dart defines', - () async { - fileSystem.file('.dart_tool/package_config.json') - ..createSync(recursive: true) - ..writeAsStringSync('{"configVersion": 2, "packages":[]}'); - final String build = iosEnvironment.buildDir.path; - final String flutterPatchedSdkPath = artifacts.getArtifactPath( - Artifact.flutterPatchedSdkPath, - platform: TargetPlatform.darwin, - mode: BuildMode.debug, - ); - processManager.addCommands([ - FakeCommand( - command: [ - artifacts.getArtifactPath(Artifact.engineDartAotRuntime), - artifacts.getArtifactPath(Artifact.frontendServerSnapshotForEngineDartSdk), - '--sdk-root', - '$flutterPatchedSdkPath/', - '--target=flutter', - '--no-print-incremental-dependencies', - '-D$kAppFlavor=vanilla', - ...buildModeOptions(BuildMode.debug, []), - '--packages', - '/.dart_tool/package_config.json', - '--output-dill', - '$build/app.dill', - '--depfile', - '$build/kernel_snapshot_program.d', - '--incremental', - '--initialize-from-dill', - '$build/app.dill', - '--verbosity=error', - 'file:///lib/main.dart', - ], - stdout: 'result $kBoundaryKey\n$kBoundaryKey\n$kBoundaryKey $build/app.dill 0\n', - ), - ]); - - await const KernelSnapshot().build( - iosEnvironment - ..defines[kTargetPlatform] = getNameForTargetPlatform(TargetPlatform.darwin) - ..defines[kBuildMode] = BuildMode.debug.cliName - ..defines[kDartDefines] = base64Encode(utf8.encode('FLUTTER_APP_FLAVOR=vanilla')) - ..defines[kFlavor] = 'strawberry' - ..defines[kTrackWidgetCreation] = 'false', - ); - - expect(processManager, hasNoRemainingExpectations); - }, - overrides: { - Platform: () => macPlatform, - FileSystem: () => fileSystem, - ProcessManager: () => processManager, - }, - ); - testWithoutContext('KernelSnapshot does use track widget creation on debug builds', () async { fileSystem.file('.dart_tool/package_config.json') ..createSync(recursive: true) diff --git a/packages/flutter_tools/test/general.shard/runner/flutter_command_test.dart b/packages/flutter_tools/test/general.shard/runner/flutter_command_test.dart index fbda6491f99..00b603d354f 100644 --- a/packages/flutter_tools/test/general.shard/runner/flutter_command_test.dart +++ b/packages/flutter_tools/test/general.shard/runner/flutter_command_test.dart @@ -1270,98 +1270,6 @@ flutter: ); }); - testUsingContext( - "tool exits when $kAppFlavor is already set in user's environemnt", - () async { - final CommandRunner runner = createTestCommandRunner( - _TestRunCommandThatOnlyValidates(), - ); - expect( - runner.run(['run', '--no-pub', '--no-hot']), - throwsToolExit( - message: '$kAppFlavor is used by the framework and cannot be set in the environment.', - ), - ); - }, - overrides: { - DeviceManager: - () => FakeDeviceManager()..attachedDevices = [FakeDevice('name', 'id')], - FileSystem: () { - final MemoryFileSystem fileSystem = MemoryFileSystem.test(); - fileSystem.file('lib/main.dart').createSync(recursive: true); - fileSystem.file('pubspec.yaml').createSync(); - return fileSystem; - }, - ProcessManager: FakeProcessManager.empty, - Platform: () => FakePlatform()..environment = {kAppFlavor: 'AlreadySet'}, - }, - ); - - testUsingContext( - 'tool exits when $kAppFlavor is set in --dart-define', - () async { - final CommandRunner runner = createTestCommandRunner( - _TestRunCommandThatOnlyValidates(), - ); - expect( - runner.run([ - 'run', - '--dart-define=$kAppFlavor=AlreadySet', - '--no-pub', - '--no-hot', - ]), - throwsToolExit( - message: '$kAppFlavor is used by the framework and cannot be set using --dart-define', - ), - ); - }, - overrides: { - DeviceManager: - () => FakeDeviceManager()..attachedDevices = [FakeDevice('name', 'id')], - FileSystem: () { - final MemoryFileSystem fileSystem = MemoryFileSystem.test(); - fileSystem.file('lib/main.dart').createSync(recursive: true); - fileSystem.file('pubspec.yaml').createSync(); - return fileSystem; - }, - ProcessManager: FakeProcessManager.empty, - }, - ); - - testUsingContext( - 'tool exits when $kAppFlavor is set in --dart-define-from-file', - () async { - final CommandRunner runner = createTestCommandRunner( - _TestRunCommandThatOnlyValidates(), - ); - expect( - runner.run([ - 'run', - '--dart-define-from-file=config.json', - '--no-pub', - '--no-hot', - ]), - throwsToolExit( - message: '$kAppFlavor is used by the framework and cannot be set using --dart-define', - ), - ); - }, - overrides: { - DeviceManager: - () => FakeDeviceManager()..attachedDevices = [FakeDevice('name', 'id')], - FileSystem: () { - final MemoryFileSystem fileSystem = MemoryFileSystem.test(); - fileSystem.file('lib/main.dart').createSync(recursive: true); - fileSystem.file('pubspec.yaml').createSync(); - fileSystem.file('config.json') - ..createSync() - ..writeAsStringSync('{"$kAppFlavor": "AlreadySet"}'); - return fileSystem; - }, - ProcessManager: FakeProcessManager.empty, - }, - ); - group('Flutter version', () { for (final String dartDefine in FlutterCommand.flutterVersionDartDefines) { testUsingContext( diff --git a/packages/flutter_tools/test/integration.shard/default_flavor_test.dart b/packages/flutter_tools/test/integration.shard/default_flavor_test.dart deleted file mode 100644 index f84008a05d9..00000000000 --- a/packages/flutter_tools/test/integration.shard/default_flavor_test.dart +++ /dev/null @@ -1,74 +0,0 @@ -// Copyright 2014 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -@Tags(['flutter-test-driver']) -library; - -import 'package:flutter_tools/src/base/file_system.dart'; - -import '../src/common.dart'; -import 'test_data/project.dart'; -import 'test_driver.dart'; -import 'test_utils.dart'; - -void main() { - final Project project = _DefaultFlavorProject(); - late Directory tempDir; - late FlutterTestTestDriver flutter; - - setUp(() async { - tempDir = createResolvedTempDirectorySync('default_flavor_test.'); - await project.setUpIn(tempDir); - flutter = FlutterTestTestDriver(tempDir); - }); - - tearDown(() async { - tryToDelete(tempDir); - }); - - testWithoutContext('Reads "default-flavor" in "flutter test"', () async { - await flutter.test(); - - // Without an assertion, this test always passes. - final int? exitCode = await flutter.done; - expect(exitCode, 0, reason: 'flutter test failed with exit code $exitCode'); - }); -} - -final class _DefaultFlavorProject extends Project { - @override - final String main = r''' - // Irrelevant to this test. - void main() {} - '''; - - @override - final String pubspec = r''' - name: test - environment: - sdk: ^3.7.0-0 - - flutter: - default-flavor: dev - - dependencies: - flutter: - sdk: flutter - dev_dependencies: - flutter_test: - sdk: flutter - '''; - - @override - final String test = r''' - import 'package:flutter/services.dart'; - import 'package:flutter_test/flutter_test.dart'; - - void main() { - test('receives default-flavor with flutter test', () async { - expect(appFlavor, 'dev'); - }); - } - '''; -} diff --git a/packages/flutter_tools/test/integration.shard/test_driver.dart b/packages/flutter_tools/test/integration.shard/test_driver.dart index 5671f310718..ee9e9d953d7 100644 --- a/packages/flutter_tools/test/integration.shard/test_driver.dart +++ b/packages/flutter_tools/test/integration.shard/test_driver.dart @@ -140,10 +140,7 @@ abstract final class FlutterTestDriver { _stderr.stream.listen((String message) => _debugPrint(message, topic: '<=stderr=')); } - /// Completes when process exits with the given exit code. - /// - /// If the process has never been started, complets with `null`. - Future get done async => _process?.exitCode; + Future get done async => _process?.exitCode; Future connectToVmService({bool pauseOnExceptions = false}) async { _vmService = await vmServiceConnectUri('$_vmServiceWsUri');