[flutter_conductor] Conductor prompt refactor (#93896)

dev/conductor/core/lib/src/context.dart

@@ -0,0 +1,54 @@
+// 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.
+
+import 'package:file/file.dart' show File;
+
+import 'globals.dart';
+import 'proto/conductor_state.pb.dart' as pb;
+import 'repository.dart';
+import 'state.dart';
+import 'stdio.dart' show Stdio;
+
+/// Interface for shared functionality across all sub-commands.
+///
+/// Different frontends (e.g. CLI vs desktop) can share [Context]s, although
+/// methods for capturing user interaction may be overridden.
+abstract class Context {
+  const Context({
+    required this.checkouts,
+    required this.stateFile,
+  });
+
+  final Checkouts checkouts;
+  final File stateFile;
+  Stdio get stdio => checkouts.stdio;
+
+  /// Confirm an action with the user before proceeding.
+  ///
+  /// The default implementation reads from STDIN. This can be overriden in UI
+  /// implementations that capture user interaction differently.
+  bool prompt(String message) {
+    stdio.write('${message.trim()} (y/n) ');
+    final String response = stdio.readLineSync().trim();
+    final String firstChar = response[0].toUpperCase();
+    if (firstChar == 'Y') {
+      return true;
+    }
+    if (firstChar == 'N') {
+      return false;
+    }
+    throw ConductorException(
+      'Unknown user input (expected "y" or "n"): $response',
+    );
+  }
+
+  /// Save the release's [state].
+  ///
+  /// This can be overridden by frontends that may not persist the state to
+  /// disk, and/or may need to call additional update hooks each time the state
+  /// is updated.
+  void updateState(pb.ConductorState state, [List<String> logs = const <String>[]]) {
+    writeStateToFile(stateFile, state, logs);
+  }
+}

dev/conductor/core/lib/src/next.dart

@@ -4,13 +4,13 @@
 
 import 'package:args/command_runner.dart';
 import 'package:file/file.dart' show File;
-import 'package:meta/meta.dart' show visibleForTesting, visibleForOverriding;
+
-import './globals.dart';
+import 'context.dart';
-import './proto/conductor_state.pb.dart' as pb;
+import 'globals.dart';
-import './proto/conductor_state.pbenum.dart';
+import 'proto/conductor_state.pb.dart' as pb;
-import './repository.dart';
+import 'proto/conductor_state.pbenum.dart';
-import './state.dart' as state_import;
+import 'repository.dart';
-import './stdio.dart';
+import 'state.dart' as state_import;
 
 const String kStateOption = 'state-file';
 const String kYesFlag = 'yes';
@@ -68,21 +68,21 @@ class NextCommand extends Command<void> {
 ///
 /// Any calls to functions that cause side effects are wrapped in methods to
 /// allow overriding in unit tests.
-class NextContext {
+class NextContext extends Context {
-  NextContext({
+  const NextContext({
     required this.autoAccept,
     required this.force,
-    required this.checkouts,
+    required Checkouts checkouts,
-    required this.stateFile,
+    required File stateFile,
-  });
+  }) : super(
+    checkouts: checkouts,
+    stateFile: stateFile,
+  );
 
   final bool autoAccept;
   final bool force;
-  final Checkouts checkouts;
-  final File stateFile;
 
   Future<void> run(pb.ConductorState state) async {
-    final Stdio stdio = checkouts.stdio;
     const List<CherrypickState> finishedStates = <CherrypickState>[
       CherrypickState.COMPLETED,
       CherrypickState.ABANDONED,
@@ -142,9 +142,8 @@ class NextContext {
         }
         if (autoAccept == false) {
           final bool response = prompt(
-              'Are you ready to push your engine branch to the repository '
+            'Are you ready to push your engine branch to the repository '
-              '${state.engine.mirror.url}?',
+            '${state.engine.mirror.url}?',
-              stdio,
           );
           if (!response) {
             stdio.printError('Aborting command.');
@@ -169,8 +168,7 @@ class NextContext {
         if (autoAccept == false) {
           // TODO(fujino): actually test if binaries have been codesigned on macOS
           final bool response = prompt(
-              'Has CI passed for the engine PR and binaries been codesigned?',
+            'Has CI passed for the engine PR and binaries been codesigned?',
-              stdio,
           );
           if (!response) {
             stdio.printError('Aborting command.');
@@ -209,14 +207,14 @@ class NextContext {
         final String engineRevision = await engine.reverseParse('HEAD');
 
         final Remote upstream = Remote(
-            name: RemoteName.upstream,
+          name: RemoteName.upstream,
-            url: state.framework.upstream.url,
+          url: state.framework.upstream.url,
         );
         final FrameworkRepository framework = FrameworkRepository(
-            checkouts,
+          checkouts,
-            initialRef: state.framework.workingBranch,
+          initialRef: state.framework.workingBranch,
-            upstreamRemote: upstream,
+          upstreamRemote: upstream,
-            previousCheckoutLocation: state.framework.checkoutPath,
+          previousCheckoutLocation: state.framework.checkoutPath,
         );
 
         // Check if the current candidate branch is enabled
@@ -277,9 +275,8 @@ class NextContext {
 
         if (autoAccept == false) {
           final bool response = prompt(
-              'Are you ready to push your framework branch to the repository '
+            'Are you ready to push your framework branch to the repository '
-              '${state.framework.mirror.url}?',
+            '${state.framework.mirror.url}?',
-              stdio,
           );
           if (!response) {
             stdio.printError('Aborting command.');
@@ -289,10 +286,10 @@ class NextContext {
         }
 
         await framework.pushRef(
-            fromRef: 'HEAD',
+          fromRef: 'HEAD',
-            // Explicitly create new branch
+          // Explicitly create new branch
-            toRef: 'refs/heads/${state.framework.workingBranch}',
+          toRef: 'refs/heads/${state.framework.workingBranch}',
-            remote: state.framework.mirror.name,
+          remote: state.framework.mirror.name,
         );
         break;
       case pb.ReleasePhase.PUBLISH_VERSION:
@@ -312,9 +309,8 @@ class NextContext {
         final String headRevision = await framework.reverseParse('HEAD');
         if (autoAccept == false) {
           final bool response = prompt(
-              'Are you ready to tag commit $headRevision as ${state.releaseVersion}\n'
+            'Are you ready to tag commit $headRevision as ${state.releaseVersion}\n'
-              'and push to remote ${state.framework.upstream.url}?',
+            'and push to remote ${state.framework.upstream.url}?',
-              stdio,
           );
           if (!response) {
             stdio.printError('Aborting command.');
@@ -347,10 +343,7 @@ class NextContext {
               dryRun: true,
           );
 
-          final bool response = prompt(
+          final bool response = prompt('Are you ready to publish this release?');
-              'Are you ready to publish this release?',
-              stdio,
-          );
           if (!response) {
             stdio.printError('Aborting command.');
             updateState(state, stdio.logs);
@@ -370,10 +363,7 @@ class NextContext {
             '\t$kLuciPackagingConsoleLink',
         );
         if (autoAccept == false) {
-          final bool response = prompt(
+          final bool response = prompt('Have all packaging builds finished successfully?');
-              'Have all packaging builds finished successfully?',
-              stdio,
-          );
           if (!response) {
             stdio.printError('Aborting command.');
             updateState(state, stdio.logs);
@@ -391,30 +381,4 @@ class NextContext {
 
     updateState(state, stdio.logs);
   }
-
-  /// Save the release's [state].
-  ///
-  /// This can be overridden by frontends that may not persist the state to
-  /// disk, and/or may need to call additional update hooks each time the state
-  /// is updated.
-  @visibleForOverriding
-  void updateState(pb.ConductorState state, [List<String> logs = const <String>[]]) {
-    state_import.writeStateToFile(stateFile, state, logs);
-  }
-
-  @visibleForTesting
-  bool prompt(String message, Stdio stdio) {
-    stdio.write('${message.trim()} (y/n) ');
-    final String response = stdio.readLineSync().trim();
-    final String firstChar = response[0].toUpperCase();
-    if (firstChar == 'Y') {
-      return true;
-    }
-    if (firstChar == 'N') {
-      return false;
-    }
-    throw ConductorException(
-      'Unknown user input (expected "y" or "n"): $response',
-    );
-  }
 }

dev/conductor/core/lib/src/start.dart

@@ -6,18 +6,18 @@ import 'package:args/args.dart';
 import 'package:args/command_runner.dart';
 import 'package:file/file.dart';
 import 'package:fixnum/fixnum.dart';
-import 'package:meta/meta.dart' show visibleForOverriding;
 import 'package:platform/platform.dart';
 import 'package:process/process.dart';
 
-import './git.dart';
+import 'context.dart';
-import './globals.dart';
+import 'git.dart';
-import './proto/conductor_state.pb.dart' as pb;
+import 'globals.dart';
-import './proto/conductor_state.pbenum.dart' show ReleasePhase;
+import 'proto/conductor_state.pb.dart' as pb;
-import './repository.dart';
+import 'proto/conductor_state.pbenum.dart' show ReleasePhase;
-import './state.dart' as state_import;
+import 'repository.dart';
-import './stdio.dart';
+import 'state.dart' as state_import;
-import './version.dart';
+import 'stdio.dart';
+import 'version.dart';
 
 const String kCandidateOption = 'candidate-branch';
 const String kDartRevisionOption = 'dart-revision';
@@ -207,7 +207,6 @@ class StartCommand extends Command<void> {
       processManager: processManager,
       releaseChannel: releaseChannel,
       stateFile: stateFile,
-      stdio: stdio,
       force: force,
     );
     return context.run();
@@ -217,10 +216,9 @@ class StartCommand extends Command<void> {
 /// Context for starting a new release.
 ///
 /// This is a frontend-agnostic implementation.
-class StartContext {
+class StartContext extends Context {
   StartContext({
     required this.candidateBranch,
-    required this.checkouts,
     required this.dartRevision,
     required this.engineCherrypickRevisions,
     required this.engineMirror,
@@ -232,13 +230,17 @@ class StartContext {
     required this.incrementLetter,
     required this.processManager,
     required this.releaseChannel,
-    required this.stateFile,
+    required Checkouts checkouts,
-    required this.stdio,
+    required File stateFile,
     this.force = false,
-  }) : git = Git(processManager);
+  }) :
+    git = Git(processManager),
+    super(
+      checkouts: checkouts,
+      stateFile: stateFile,
+    );
 
   final String candidateBranch;
-  final Checkouts checkouts;
   final String? dartRevision;
   final List<String> engineCherrypickRevisions;
   final String engineMirror;
@@ -251,8 +253,6 @@ class StartContext {
   final Git git;
   final ProcessManager processManager;
   final String releaseChannel;
-  final File stateFile;
-  final Stdio stdio;
 
   /// If validations should be overridden.
   final bool force;
@@ -410,16 +410,6 @@ class StartContext {
     stdio.printStatus(state_import.presentState(state));
   }
 
-  /// Save the release's [state].
-  ///
-  /// This can be overridden by frontends that may not persist the state to
-  /// disk, and/or may need to call additional update hooks each time the state
-  /// is updated.
-  @visibleForOverriding
-  void updateState(pb.ConductorState state, List<String> logs) {
-    state_import.writeStateToFile(stateFile, state, logs);
-  }
-
   /// Determine this release's version number from the [lastVersion] and the [incrementLetter].
   Version calculateNextVersion(Version lastVersion) {
     if (incrementLetter == 'm') {
@@ -457,7 +447,18 @@ class StartContext {
       candidateBranch,
       FrameworkRepository.defaultBranch,
     );
+    final bool response = prompt(
+      'About to tag the release candidate branch branchpoint of $branchPoint '
+      'as $requestedVersion and push it to ${framework.upstreamRemote.url}. '
+      'Is this correct?',
+    );
+
+    if (!response) {
+      throw ConductorException('Aborting command.');
+    }
+
     stdio.printStatus('Applying the tag $requestedVersion at the branch point $branchPoint');
+
     await framework.tag(
       branchPoint,
       requestedVersion.toString(),

dev/conductor/core/test/common.dart

@@ -34,7 +34,7 @@ class TestStdio extends Stdio {
   TestStdio({
     this.verbose = false,
     List<String>? stdin,
-  }) : _stdin = stdin ?? <String>[];
+  }) : stdin = stdin ?? <String>[];
 
   String get error => logs.where((String log) => log.startsWith(r'[error] ')).join('\n');
 
@@ -43,15 +43,14 @@ class TestStdio extends Stdio {
   }).join('\n');
 
   final bool verbose;
-  late final List<String> _stdin;
+  final List<String> stdin;
-  List<String> get stdin => _stdin;
 
   @override
   String readLineSync() {
-    if (_stdin.isEmpty) {
+    if (stdin.isEmpty) {
       throw Exception('Unexpected call to readLineSync!');
     }
-    return _stdin.removeAt(0);
+    return stdin.removeAt(0);
   }
 }
 

dev/conductor/core/test/next_test.dart

@@ -8,6 +8,7 @@ import 'package:conductor_core/src/proto/conductor_state.pb.dart' as pb;
 import 'package:conductor_core/src/proto/conductor_state.pbenum.dart' show ReleasePhase;
 import 'package:conductor_core/src/repository.dart';
 import 'package:conductor_core/src/state.dart';
+import 'package:conductor_core/src/stdio.dart';
 import 'package:file/file.dart';
 import 'package:file/memory.dart';
 import 'package:platform/platform.dart';
@@ -1054,6 +1055,27 @@ void main() {
   });
 
   group('prompt', () {
+    test('can be overridden for different frontend implementations', () {
+      final FileSystem fileSystem = MemoryFileSystem.test();
+      final Stdio stdio = _UnimplementedStdio.instance;
+      final Checkouts checkouts = Checkouts(
+        fileSystem: fileSystem,
+        parentDirectory: fileSystem.directory('/'),
+        platform: FakePlatform(),
+        processManager: FakeProcessManager.empty(),
+        stdio: stdio,
+      );
+      final _TestNextContext context = _TestNextContext(
+        checkouts: checkouts,
+        stateFile: fileSystem.file('/statefile.json'),
+      );
+
+      final bool response = context.prompt(
+        'A prompt that will immediately be agreed to',
+      );
+      expect(response, true);
+    });
+
     test('throws if user inputs character that is not "y" or "n"', () {
       final FileSystem fileSystem = MemoryFileSystem.test();
       final TestStdio stdio = TestStdio(
@@ -1075,13 +1097,64 @@ void main() {
       );
 
       expect(
-        () => context.prompt('Asking a question?', stdio),
+        () => context.prompt('Asking a question?'),
         throwsExceptionWith('Unknown user input (expected "y" or "n")'),
       );
     });
   });
 }
 
+/// A [Stdio] that will throw an exception if any of its methods are called.
+class _UnimplementedStdio implements Stdio {
+  const _UnimplementedStdio();
+
+  static const _UnimplementedStdio _instance = _UnimplementedStdio();
+  static _UnimplementedStdio get instance => _instance;
+
+  Never _throw() => throw Exception('Unimplemented!');
+
+  @override
+  List<String> get logs => _throw();
+
+  @override
+  void printError(String message) => _throw();
+
+  @override
+  void printWarning(String message) => _throw();
+
+  @override
+  void printStatus(String message) => _throw();
+
+  @override
+  void printTrace(String message) => _throw();
+
+  @override
+  void write(String message) => _throw();
+
+  @override
+  String readLineSync() => _throw();
+}
+
+class _TestNextContext extends NextContext {
+  const _TestNextContext({
+    bool autoAccept = false,
+    bool force = false,
+    required File stateFile,
+    required Checkouts checkouts,
+  }) : super(
+    autoAccept: autoAccept,
+    force: force,
+    checkouts: checkouts,
+    stateFile: stateFile,
+  );
+
+  @override
+  bool prompt(String message) {
+    // always say yes
+    return true;
+  }
+}
+
 void _initializeCiYamlFile(
   File file, {
   List<String>? enabledBranches,

dev/conductor/core/test/start_test.dart

@@ -306,6 +306,7 @@ void main() {
     });
 
     test('creates state file if provided correct inputs', () async {
+      stdio.stdin.add('y'); // accept prompt from ensureBranchPointTagged()
       const String revision2 = 'def789';
       const String revision3 = '123abc';
       const String branchPointRevision='deadbeef';