Add --replay-from argument to Flutter tools #7146

tvolkert · 2016-12-04T00:01:18Z

This argument will enable mocking of os-layer process invocations,
where the mock behavior will come from replaying a previously-
recorded set of invocations. At the point of process invocation,
the key metadata for the invocation will be looked up in the
recording's manifest, and iff a matching record exists in the
menifest, the process will be mocked out with data derived from
the corresponding recorded process (e.g. stdout, stderr, ecit code).

This argument will enable mocking of os-layer process invocations, where the mock behavior will come from replaying a previously- recorded set of invocations. At the point of process invocation, the key metadata for the invocation will be looked up in the recording's manifest, and iff a matching record exists in the menifest, the process will be mocked out with data derived from the corresponding recorded process (e.g. stdout, stderr, ecit code).

tvolkert · 2016-12-04T00:01:40Z

@cbracken @danrubel @johnmccutchan

tvolkert · 2016-12-04T15:21:32Z

FYI, again, you need to click "Load diff" to see the full changes in this PR...

danrubel

LGTM with comments for you to address as you see fit.

danrubel · 2016-12-05T16:52:54Z

packages/flutter_tools/lib/src/runner/flutter_command_runner.dart

-            : new File(recordToPath);
-      }
+      // Turn on recording.
+      assert(globalResults['replay-from'] == null);


Perhaps replace thisassert and the one below with a check earlier in the code that indicates to the user that they can't use both options together...

if (globalResults['record-to'] != null && globalResults['replay-from'] == null) throw new ToolExit('Cannot use --record-to and --replay-from options at the same time');

danrubel · 2016-12-05T16:53:34Z

packages/flutter_tools/lib/src/runner/flutter_command_runner.dart

+          globalResults['replay-from'].trim(),
+        ));
+      } on ArgumentError catch (_) {
+        printError('--replay-from must specify a valid file or directory.');


throw new ToolExit instead?

danrubel · 2016-12-05T17:06:02Z

packages/flutter_tools/lib/src/base/process_manager.dart

+      : pid = result.pid,
+        _stdout = result.stdout,
+        _stderr = result.stderr,
+        _stdoutController = new StreamController<List<int>>.broadcast(),


Are Process stdout/stderr broadcast streams? If not, will there be a race condition or will some stdout/stderr data fall on the floor during replay if these are broadcast streams? I'm not sure of the answer myself...

Good catch. No, they're not broadcast - fixed.

danrubel · 2016-12-05T17:09:28Z

packages/flutter_tools/lib/src/base/process_manager.dart

+    // Produce stream events after a delay so that all interested parties have
+    // a chance to add themselves as listeners before we fire our stream events
+    // and terminate the process.
+    new Timer(const Duration(milliseconds: 50), () {


Ah... maybe this delay before adding data to stdout/stderr alleviates the issue I raised above. I prefer not to have arbitrary delays in the code, but this should work.

I left this timeout in (for now) and updated the comment to reflect exactly why it's needed. I plan to keep digging to see if I can solve it a better way, but I can do that in a follow-on PR.

cbracken

cbracken · 2016-12-05T17:15:33Z

packages/flutter_tools/lib/src/base/process_manager.dart

  bool kill([ProcessSignal signal = ProcessSignal.SIGTERM]) => delegate.kill(signal);
 }
+
+/// a [ProcessManager] implementation that mocks out all process invocations


uber-nit: s/a/A/ here

cbracken · 2016-12-05T17:21:30Z

packages/flutter_tools/lib/src/base/process_manager.dart

+      return new ReplayProcessManager._(manifest, dir);
+    } on FormatException catch (_) {
+      throw new ArgumentError('$_kManifestName is not a valid JSON file.');
+    }


Maybe assert(false) here for future-proofing in case someone starts throwing something other than FormatException.

Discussed offline.

cbracken · 2016-12-05T17:24:17Z

packages/flutter_tools/lib/src/base/process_manager.dart

+    Encoding stdoutEncoding,
+    Encoding stderrEncoding,
+  }) {
+    ListsEqual<String> equal = const ListEquality<String>().equals;


Why not make this a private top-level const kListsEqual? If not, make final.

cbracken · 2016-12-05T17:27:47Z

packages/flutter_tools/lib/src/base/process_manager.dart

+
+  @override
+  bool killPid(int pid, [ProcessSignal signal = ProcessSignal.SIGTERM]) {
+    throw new UnimplementedError();


UnimplementedError or UnsupportedError? If Unimplemented, add a TODO.

cbracken · 2016-12-05T17:33:07Z

packages/flutter_tools/lib/src/base/process_manager.dart

+
+  static Future<dynamic> _getData(String path, String encoding) async {
+    File file = new File(path);
+    assert(await file.exists());


I'd kill the assert here and below. This feels more like a legit case for a runtime exception (e.g., FileSystemException which you'll get from the read calls below) than a logical invariant violation on the part of the programmer. Filesystems can get unmounted at runtime, etc.

tvolkert · 2016-12-05T17:53:13Z

Ok, all comments addressed.

johnmccutchan

LGTM with some comments

johnmccutchan · 2016-12-05T20:58:54Z

packages/flutter_tools/lib/src/base/process_manager.dart

+/// Recordings are expected to be of the form produced by
+/// [RecordingProcessManager]. Namely, this includes:
+///
+/// - a [_kManifestName](manifest file) encoded as UTF-8 JSON that lists all


doc commenting a private member might not work?

In practice we don't actually publish the flutter tools docs anyway. They're for internal use basically. We just use dartdoc format for consistency.

johnmccutchan · 2016-12-05T21:00:51Z

packages/flutter_tools/lib/src/base/process_manager.dart

+        dir = new Directory(location);
+        break;
+      case FileSystemEntityType.NOT_FOUND:
+        throw new ArgumentError.value(location, 'location');


ArgumentError takes a message parameter, you could use it to help make this failure case clearer.

johnmccutchan · 2016-12-05T21:01:12Z

packages/flutter_tools/lib/src/base/process_manager.dart

+      // valid replay directory. Namely, we don't validate the structure of the
+      // JSON within the manifest, and we don't validate the existence of
+      // all stdout and stderr files referenced in the manifest.
+      throw new ArgumentError('$_kManifestName not found.');


johnmccutchan · 2016-12-05T21:04:44Z

packages/flutter_tools/lib/src/base/process_manager.dart

+    Map<String, dynamic> entry,
+  ) async {
+    String basePath = path.join(dir.path, entry['basename']);
+    return new _ReplayProcessResult._(


The failure path here where _getData throws an exception (because stdout or stderr can't be found) might bubble up to the callers. Consider catching here and throwing whatever error Process throws when it can't spawn a process.

johnmccutchan · 2016-12-05T21:05:10Z

packages/flutter_tools/lib/src/base/process_manager.dart

+    // prevents the case where our device log reader consumes all stdio before
+    // we've even started the app, thus causing the protocol discovery process
+    // to fail.
+    new Timer(const Duration(milliseconds: 50), () {


please add a TODO here and a comment about this being temporary.

Hixie · 2016-12-05T21:09:46Z

Test please.

Hixie · 2016-12-05T21:10:15Z

packages/flutter_tools/pubspec.yaml

 dependencies:
  archive: ^1.0.20
  args: ^0.13.4
+  collection: '>=1.9.1 <2.0.0'


Is there any way to get out of having this dependency? We've been trying to drop our dependency on package:collection...

Hixie · 2016-12-05T21:10:58Z

packages/flutter_tools/lib/src/runner/flutter_command_runner.dart

+    argParser.addOption('replay-from',
+        help:
+            'Enables mocking of process invocations by replaying their stdout '
+            ', stderr, and exit code from the specified recording (obtained '


typo (stdout , stderr,, extra space before the first comma)

Hixie · 2016-12-05T21:13:21Z

packages/flutter_tools/lib/src/base/process_manager.dart

+  }
+
+  @override
+  Future<Process> start(


nit: please match flutter style for these

Hixie · 2016-12-05T21:14:46Z

packages/flutter_tools/lib/src/base/process_manager.dart

  }) {
    Map<String, dynamic> entry = <String, dynamic>{};
    if (pid != null) entry['pid'] = pid;
+    if (basename != null) entry['basename'] = basename;


please change these ifs to have their bodies on a separate line

Hixie · 2016-12-05T21:15:01Z

packages/flutter_tools/lib/src/base/process_manager.dart

  /// process invocation.
  Map<String, dynamic> _createManifestEntry({
      int pid,
+      String basename,


nit: these should be two-character indents not 4

Hixie · 2016-12-05T21:17:08Z

packages/flutter_tools/lib/src/base/process_manager.dart

+typedef bool ListsEqual<T>(List<T> list1, List<T> list2);
+
+const String _kManifestName = 'MANIFEST.txt';
+final ListsEqual<String> kListsEqual = const ListEquality<String>().equals;


rather than using collection.dart, just have a function somewhere in flutter_tool that does this (if you use it a lot) or just inline it (if not).

if (list1.length != list2.length) return false; for (int index = 0; index < list1.length; index += 1) { if (list1[index] != list2[index) return false; } return true;

tvolkert · 2016-12-06T18:08:17Z

All comments have been addressed, modulo adding a test (which will come in a follow-on change). Merging...

devoncarew · 2016-12-13T21:21:57Z

@tvolkert, I notice that flutter -h is now printing out args for --record-to and --replay-from. I assume these are less common flags for flutter users to use? If so, it'd suggest hiding them be default, to keep the help text focused on the fewer, more likely options. You can add hide: !verboseHelp as a named param when you define the option.

googlebot added the cla: yes label Dec 4, 2016

Fixed Travis analyzer errors

c19ca33

tvolkert added 5 commits December 4, 2016 21:28

Bug fixes

ea8f412

Cleanup

608dc85

Restructure how the replay process produces stream events

67e2557

Remove file that I accidentally added

7e5173d

Self-code-review

574d516

danrubel approved these changes Dec 5, 2016

View reviewed changes

cbracken approved these changes Dec 5, 2016

View reviewed changes

tvolkert added 2 commits December 5, 2016 09:39

Review comments from @danrubel

56c30ee

Review comments by @cbracken

5da13f3

kListsEqual can't be const

f1fce54

johnmccutchan approved these changes Dec 5, 2016

View reviewed changes

Hixie reviewed Dec 5, 2016

View reviewed changes

tvolkert added 2 commits December 6, 2016 09:43

Review comments

fabfdaf

Removed test file accidentally pushed

e5f5931

tvolkert merged commit 7536404 into flutter:master Dec 6, 2016

tvolkert deleted the replay branch December 6, 2016 18:09

github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2021

Add --replay-from argument to Flutter tools #7146

Add --replay-from argument to Flutter tools #7146

Uh oh!

Conversation

tvolkert commented Dec 4, 2016

Uh oh!

tvolkert commented Dec 4, 2016

Uh oh!

tvolkert commented Dec 4, 2016

Uh oh!

danrubel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvolkert commented Dec 5, 2016

Uh oh!

johnmccutchan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Dec 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment