refactor(shorebird_cli): throw error if incomplete engine config args are provided (#1616)

Co-authored-by: Felix Angelov <felix@shorebird.dev>

packages/shorebird_cli/bin/shorebird.dart

@@ -9,6 +9,7 @@ import 'package:shorebird_cli/src/cache.dart';
 import 'package:shorebird_cli/src/code_push_client_wrapper.dart';
 import 'package:shorebird_cli/src/command_runner.dart';
 import 'package:shorebird_cli/src/doctor.dart';
+import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/executables/executables.dart';
 import 'package:shorebird_cli/src/http_client/http_client.dart';
 import 'package:shorebird_cli/src/logger.dart';

packages/shorebird_cli/lib/src/command_runner.dart

@@ -6,6 +6,7 @@ import 'package:cli_completion/cli_completion.dart';
 import 'package:mason_logger/mason_logger.dart';
 import 'package:scoped/scoped.dart';
 import 'package:shorebird_cli/src/commands/commands.dart';
+import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/logger.dart';
 import 'package:shorebird_cli/src/platform.dart';
 import 'package:shorebird_cli/src/process.dart';
@@ -86,13 +87,37 @@ class ShorebirdCliCommandRunner extends CompletionCommandRunner<int> {
     try {
       final topLevelResults = parse(args);
 
-      // Set up our context before running the command.
-      final engineConfig = EngineConfig(
-        localEngineSrcPath: topLevelResults['local-engine-src-path'] as String?,
-        localEngine: topLevelResults['local-engine'] as String?,
-        localEngineHost: topLevelResults['local-engine-host'] as String?,
-      );
-      final process = ShorebirdProcess(engineConfig: engineConfig);
+      final localEngineSrcPath =
+          topLevelResults['local-engine-src-path'] as String?;
+      final localEngine = topLevelResults['local-engine'] as String?;
+      final localEngineHost = topLevelResults['local-engine-host'] as String?;
+
+      final localEngineArgs = [
+        localEngineSrcPath,
+        localEngine,
+        localEngineHost,
+      ];
+      final localEngineArgsAreNull =
+          localEngineArgs.every((arg) => arg == null);
+      final localEngineArgsAreNotNull =
+          localEngineArgs.every((arg) => arg != null);
+      final EngineConfig engineConfig;
+      if (localEngineArgsAreNotNull) {
+        engineConfig = EngineConfig(
+          localEngineSrcPath: localEngineSrcPath,
+          localEngine: localEngine,
+          localEngineHost: localEngineHost,
+        );
+      } else if (localEngineArgsAreNull) {
+        engineConfig = const EngineConfig.empty();
+      } else {
+        // Only some local engine args were provided, this is invalid.
+        throw ArgumentError(
+          '''local-engine, local-engine-src, and local-engine-host must all be provided''',
+        );
+      }
+
+      final process = ShorebirdProcess();
       final shorebirdArtifacts = engineConfig.localEngineSrcPath != null
           ? const ShorebirdLocalEngineArtifacts()
           : const ShorebirdCachedArtifacts();

packages/shorebird_cli/lib/src/commands/patch/patch_ios_command.dart

@@ -13,12 +13,12 @@ import 'package:shorebird_cli/src/command.dart';
 import 'package:shorebird_cli/src/config/config.dart';
 import 'package:shorebird_cli/src/deployment_track.dart';
 import 'package:shorebird_cli/src/doctor.dart';
+import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/executables/executables.dart';
 import 'package:shorebird_cli/src/formatters/file_size_formatter.dart';
 import 'package:shorebird_cli/src/ios.dart';
 import 'package:shorebird_cli/src/logger.dart';
 import 'package:shorebird_cli/src/patch_diff_checker.dart';
-import 'package:shorebird_cli/src/process.dart';
 import 'package:shorebird_cli/src/shorebird_artifact_mixin.dart';
 import 'package:shorebird_cli/src/shorebird_artifacts.dart';
 import 'package:shorebird_cli/src/shorebird_build_mixin.dart';

packages/shorebird_cli/lib/src/engine_config.dart

@@ -0,0 +1,29 @@
+import 'package:scoped/scoped.dart';
+
+// A reference to a [EngineConfig] instance.
+final engineConfigRef = create(() => const EngineConfig.empty());
+
+// The [EngineConfig] instance available in the current zone.
+EngineConfig get engineConfig => read(engineConfigRef);
+
+class EngineConfig {
+  const EngineConfig({
+    required this.localEngineSrcPath,
+    required this.localEngine,
+    required this.localEngineHost,
+  });
+
+  const EngineConfig.empty()
+      : localEngineSrcPath = null,
+        localEngine = null,
+        localEngineHost = null;
+
+  final String? localEngineSrcPath;
+  final String? localEngine;
+  final String? localEngineHost;
+
+  @override
+  String toString() {
+    return '''EngineConfig(localEngineSrcPath: $localEngineSrcPath, localEngine: $localEngine, localEngineHost: $localEngineHost)''';
+  }
+}

packages/shorebird_cli/lib/src/process.dart

@@ -3,32 +3,10 @@ import 'dart:io';
 import 'package:mason_logger/mason_logger.dart';
 import 'package:meta/meta.dart';
 import 'package:scoped/scoped.dart';
+import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/logger.dart';
 import 'package:shorebird_cli/src/shorebird_env.dart';
 
-// A reference to a [EngineConfig] instance.
-final engineConfigRef = create(() => const EngineConfig.empty());
-
-// The [EngineConfig] instance available in the current zone.
-EngineConfig get engineConfig => read(engineConfigRef);
-
-class EngineConfig {
-  const EngineConfig({
-    required this.localEngineSrcPath,
-    required this.localEngine,
-    required this.localEngineHost,
-  });
-
-  const EngineConfig.empty()
-      : localEngineSrcPath = null,
-        localEngine = null,
-        localEngineHost = null;
-
-  final String? localEngineSrcPath;
-  final String? localEngine;
-  final String? localEngineHost;
-}
-
 // A reference to a [ShorebirdProcess] instance.
 final processRef = create(ShorebirdProcess.new);
 
@@ -41,12 +19,10 @@ ShorebirdProcess get process => read(processRef);
 // "ProcessFactory" than a "Process".
 class ShorebirdProcess {
   ShorebirdProcess({
-    this.engineConfig = const EngineConfig.empty(),
     ProcessWrapper? processWrapper, // For mocking ShorebirdProcess.
   }) : processWrapper = processWrapper ?? ProcessWrapper();
 
   final ProcessWrapper processWrapper;
-  final EngineConfig engineConfig;
 
   Future<ShorebirdProcessResult> run(
     String executable,

packages/shorebird_cli/lib/src/shorebird_artifacts.dart

@@ -5,7 +5,7 @@ import 'dart:io';
 import 'package:path/path.dart' as p;
 import 'package:scoped/scoped.dart';
 import 'package:shorebird_cli/src/cache.dart';
-import 'package:shorebird_cli/src/process.dart';
+import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/shorebird_env.dart';
 
 /// All Shorebird artifacts used explicitly by Shorebird.

packages/shorebird_cli/lib/src/shorebird_build_mixin.dart

@@ -3,6 +3,7 @@ import 'dart:io';
 import 'package:collection/collection.dart';
 import 'package:mason_logger/mason_logger.dart';
 import 'package:shorebird_cli/src/command.dart';
+import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/logger.dart';
 import 'package:shorebird_cli/src/os/operating_system_interface.dart';
 import 'package:shorebird_cli/src/process.dart';

packages/shorebird_cli/test/src/command_runner_test.dart

@@ -236,6 +236,44 @@ Run ${lightCyan.wrap('shorebird upgrade')} to upgrade.'''),
       });
     });
 
+    group('local engine', () {
+      group('when all local engine args are provided', () {
+        test('creates engine config with arguments', () async {
+          final result = await runWithOverrides(
+            () => commandRunner.run([
+              '--local-engine',
+              'foo',
+              '--local-engine-src-path',
+              'bar',
+              '--local-engine-host',
+              'baz',
+            ]),
+          );
+          expect(result, equals(ExitCode.success.code));
+        });
+      });
+
+      group('when no local engine args are provided', () {
+        test('uses empty engine config', () async {
+          final result = await runWithOverrides(
+            () => commandRunner.run([]),
+          );
+          expect(result, equals(ExitCode.success.code));
+        });
+      });
+
+      group('when some local engine args are provided', () {
+        test('throws ArgumentException', () async {
+          await expectLater(
+            () async => runWithOverrides(
+              () => commandRunner.run(['--local-engine', 'foo']),
+            ),
+            throwsArgumentError,
+          );
+        });
+      });
+    });
+
     group('completion', () {
       test('fast tracks completion', () async {
         final result = await runWithOverrides(

packages/shorebird_cli/test/src/commands/build/build_app_bundle_command_test.dart

@@ -5,6 +5,7 @@ import 'package:path/path.dart' as p;
 import 'package:scoped/scoped.dart';
 import 'package:shorebird_cli/src/commands/build/build.dart';
 import 'package:shorebird_cli/src/doctor.dart';
+import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/logger.dart';
 import 'package:shorebird_cli/src/os/operating_system_interface.dart';
 import 'package:shorebird_cli/src/process.dart';
@@ -203,7 +204,7 @@ ${lightCyan.wrap(p.join('build', 'app', 'outputs', 'bundle', '${flavor}Release',
           () => command.architectures.length,
           values: {
             engineConfigRef.overrideWith(
-              () => const EngineConfig(
+              () => EngineConfig(
                 localEngine: 'android_release_arm64',
                 localEngineSrcPath: 'path/to/engine/src',
                 localEngineHost: 'host_release',
@@ -220,7 +221,7 @@ ${lightCyan.wrap(p.join('build', 'app', 'outputs', 'bundle', '${flavor}Release',
           () => command.architectures.length,
           values: {
             engineConfigRef.overrideWith(
-              () => const EngineConfig(
+              () => EngineConfig(
                 localEngine: 'android_debug_unopt',
                 localEngineSrcPath: 'path/to/engine/src',
                 localEngineHost: 'host_debug_unopt',

packages/shorebird_cli/test/src/commands/patch/patch_aar_command_test.dart

@@ -15,6 +15,7 @@ import 'package:shorebird_cli/src/code_push_client_wrapper.dart';
 import 'package:shorebird_cli/src/commands/commands.dart';
 import 'package:shorebird_cli/src/config/config.dart';
 import 'package:shorebird_cli/src/deployment_track.dart';
+import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/http_client/http_client.dart';
 import 'package:shorebird_cli/src/logger.dart';
 import 'package:shorebird_cli/src/os/operating_system_interface.dart';

packages/shorebird_cli/test/src/commands/patch/patch_android_command_test.dart

@@ -16,6 +16,7 @@ import 'package:shorebird_cli/src/commands/patch/patch_android_command.dart';
 import 'package:shorebird_cli/src/config/config.dart';
 import 'package:shorebird_cli/src/deployment_track.dart';
 import 'package:shorebird_cli/src/doctor.dart';
+import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/executables/executables.dart';
 import 'package:shorebird_cli/src/http_client/http_client.dart';
 import 'package:shorebird_cli/src/logger.dart';

packages/shorebird_cli/test/src/commands/patch/patch_ios_command_test.dart

@@ -16,6 +16,7 @@ import 'package:shorebird_cli/src/commands/patch/patch.dart';
 import 'package:shorebird_cli/src/config/config.dart';
 import 'package:shorebird_cli/src/deployment_track.dart';
 import 'package:shorebird_cli/src/doctor.dart';
+import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/executables/aot_tools.dart';
 import 'package:shorebird_cli/src/logger.dart';
 import 'package:shorebird_cli/src/os/operating_system_interface.dart';

packages/shorebird_cli/test/src/commands/patch/patch_ios_framework_command_test.dart

@@ -14,6 +14,7 @@ import 'package:shorebird_cli/src/commands/patch/patch.dart';
 import 'package:shorebird_cli/src/config/config.dart';
 import 'package:shorebird_cli/src/deployment_track.dart';
 import 'package:shorebird_cli/src/doctor.dart';
+import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/logger.dart';
 import 'package:shorebird_cli/src/os/operating_system_interface.dart';
 import 'package:shorebird_cli/src/patch_diff_checker.dart';
@@ -83,6 +84,7 @@ flutter:
     late Directory shorebirdRoot;
     late Directory projectRoot;
     late Directory flutterDirectory;
+    late EngineConfig engineConfig;
     late File genSnapshotFile;
     late ShorebirdArtifacts shorebirdArtifacts;
     late Doctor doctor;
@@ -111,6 +113,7 @@ flutter:
           authRef.overrideWith(() => auth),
           codePushClientWrapperRef.overrideWith(() => codePushClientWrapper),
           doctorRef.overrideWith(() => doctor),
+          engineConfigRef.overrideWith(() => engineConfig),
           shorebirdArtifactsRef.overrideWith(() => shorebirdArtifacts),
           loggerRef.overrideWith(() => logger),
           osInterfaceRef.overrideWith(() => operatingSystemInterface),
@@ -179,6 +182,7 @@ flutter:
       artifactManager = MockArtifactManager();
       codePushClientWrapper = MockCodePushClientWrapper();
       doctor = MockDoctor();
+      engineConfig = const EngineConfig.empty();
       shorebirdArtifacts = MockShorebirdArtifacts();
       patchDiffChecker = MockPatchDiffChecker();
       platform = MockPlatform();

packages/shorebird_cli/test/src/commands/release/release_aar_command_test.dart

@@ -11,6 +11,7 @@ import 'package:shorebird_cli/src/auth/auth.dart';
 import 'package:shorebird_cli/src/code_push_client_wrapper.dart';
 import 'package:shorebird_cli/src/commands/commands.dart';
 import 'package:shorebird_cli/src/config/config.dart';
+import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/executables/executables.dart';
 import 'package:shorebird_cli/src/logger.dart';
 import 'package:shorebird_cli/src/os/operating_system_interface.dart';

packages/shorebird_cli/test/src/commands/release/release_android_command_test.dart

@@ -13,6 +13,7 @@ import 'package:shorebird_cli/src/code_push_client_wrapper.dart';
 import 'package:shorebird_cli/src/commands/commands.dart';
 import 'package:shorebird_cli/src/config/config.dart';
 import 'package:shorebird_cli/src/doctor.dart';
+import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/executables/executables.dart';
 import 'package:shorebird_cli/src/logger.dart';
 import 'package:shorebird_cli/src/os/operating_system_interface.dart';

packages/shorebird_cli/test/src/engine_config_test.dart

@@ -0,0 +1,20 @@
+import 'package:shorebird_cli/src/engine_config.dart';
+import 'package:test/test.dart';
+
+void main() {
+  group(EngineConfig, () {
+    test('toString', () {
+      const config = EngineConfig(
+        localEngineSrcPath: 'a',
+        localEngine: 'b',
+        localEngineHost: 'c',
+      );
+      expect(
+        config.toString(),
+        equals(
+          '''EngineConfig(localEngineSrcPath: a, localEngine: b, localEngineHost: c)''',
+        ),
+      );
+    });
+  });
+}

packages/shorebird_cli/test/src/mocks.dart

@@ -16,6 +16,7 @@ import 'package:shorebird_cli/src/cache.dart' show Cache;
 import 'package:shorebird_cli/src/code_push_client_wrapper.dart';
 import 'package:shorebird_cli/src/config/config.dart';
 import 'package:shorebird_cli/src/doctor.dart';
+import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/executables/devicectl/apple_device.dart';
 import 'package:shorebird_cli/src/executables/executables.dart';
 import 'package:shorebird_cli/src/os/os.dart';

packages/shorebird_cli/test/src/shorebird_artifacts_test.dart

@@ -4,7 +4,7 @@ import 'package:mocktail/mocktail.dart';
 import 'package:path/path.dart' as p;
 import 'package:scoped/scoped.dart';
 import 'package:shorebird_cli/src/cache.dart';
-import 'package:shorebird_cli/src/process.dart';
+import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/shorebird_artifacts.dart';
 import 'package:shorebird_cli/src/shorebird_env.dart';
 import 'package:test/test.dart';

packages/shorebird_cli/test/src/shorebird_process_test.dart

@@ -4,6 +4,7 @@ import 'package:mason_logger/mason_logger.dart';
 import 'package:mocktail/mocktail.dart';
 import 'package:path/path.dart' as p;
 import 'package:scoped/scoped.dart';
+import 'package:shorebird_cli/src/engine_config.dart';
 import 'package:shorebird_cli/src/logger.dart';
 import 'package:shorebird_cli/src/process.dart';
 import 'package:shorebird_cli/src/shorebird_env.dart';
@@ -17,6 +18,7 @@ void main() {
       'FLUTTER_STORAGE_BASE_URL': 'https://download.shorebird.dev',
     };
 
+    late EngineConfig engineConfig;
     late Logger logger;
     late ProcessWrapper processWrapper;
     late Process startProcess;
@@ -28,6 +30,7 @@ void main() {
       return runScoped(
         () => body(),
         values: {
+          engineConfigRef.overrideWith(() => engineConfig),
           loggerRef.overrideWith(() => logger),
           shorebirdEnvRef.overrideWith(() => shorebirdEnv),
         },
@@ -35,6 +38,7 @@ void main() {
     }
 
     setUp(() {
+      engineConfig = const EngineConfig.empty();
       logger = MockLogger();
       processWrapper = MockProcessWrapper();
       runProcessResult = MockProcessResult();
@@ -195,14 +199,14 @@ void main() {
       );
 
       test('adds local-engine arguments if set', () async {
+        engineConfig = EngineConfig(
+          localEngineSrcPath: 'path/to/engine/src',
+          localEngine: 'android_release_arm64',
+          localEngineHost: 'host_release',
+        );
         final localEngineSrcPath = p.join('path', 'to', 'engine', 'src');
         shorebirdProcess = ShorebirdProcess(
           processWrapper: processWrapper,
-          engineConfig: EngineConfig(
-            localEngineSrcPath: localEngineSrcPath,
-            localEngine: 'android_release_arm64',
-            localEngineHost: 'host_release',
-          ),
         );
 
         await runWithOverrides(() => shorebirdProcess.run('flutter', []));