[flutter_tools] refactor the IOSDevicePortForwarder and move tests out of devices_test.dart (#52772)

Updates the IOSDevicePortForwarder to no longer depend on context, or on an IOSDevice instance. Instead, it receives all necessary configuration through the constructor. Moves the IOSDevicePortForwarder to a separate file.
2020-03-17 17:58:05 -07:00
parent 6884086e5c
commit 90d6169678
4 changed files with 121 additions and 72 deletions
--- a/packages/flutter_tools/lib/src/ios/devices.dart
+++ b/packages/flutter_tools/lib/src/ios/devices.dart
@@ -7,6 +7,7 @@ import 'dart:math' as math;

 import 'package:meta/meta.dart';
 import 'package:platform/platform.dart';
+import 'package:process/process.dart';

 import '../application_package.dart';
 import '../artifacts.dart';
@@ -365,7 +366,13 @@ class IOSDevice extends Device {
  }

  @override
-  DevicePortForwarder get portForwarder => _portForwarder ??= IOSDevicePortForwarder(this);
+  DevicePortForwarder get portForwarder => _portForwarder ??= IOSDevicePortForwarder(
+    processManager: globals.processManager,
+    logger: globals.logger,
+    dyLdLibEntry: globals.cache.dyLdLibEntry,
+    id: id,
+    iproxyPath: _iproxyPath,
+  );

  @visibleForTesting
  set portForwarder(DevicePortForwarder forwarder) {
@@ -583,20 +590,56 @@ class IOSDeviceLogReader extends DeviceLogReader {
  }
 }

-@visibleForTesting
+/// A [DevicePortForwarder] specialized for iOS usage with iproxy.
 class IOSDevicePortForwarder extends DevicePortForwarder {
-  IOSDevicePortForwarder(this.device) : _forwardedPorts = <ForwardedPort>[];

-  final IOSDevice device;
+  /// Create a new [IOSDevicePortForwarder].
+  IOSDevicePortForwarder({
+    @required ProcessManager processManager,
+    @required Logger logger,
+    @required MapEntry<String, String> dyLdLibEntry,
+    @required String id,
+    @required String iproxyPath,
+  }) : _logger = logger,
+       _dyLdLibEntry = dyLdLibEntry,
+       _id = id,
+       _iproxyPath = iproxyPath,
+       _processUtils = ProcessUtils(processManager: processManager, logger: logger);

-  final List<ForwardedPort> _forwardedPorts;
+  /// Create a [IOSDevicePortForwarder] for testing.
+  ///
+  /// This specifies the path to iproxy as 'iproxy` and the dyLdLibEntry as
+  /// 'DYLD_LIBRARY_PATH: /path/to/libs'.
+  ///
+  /// The device id may be provided, but otherwise defaultts to '1234'.
+  factory IOSDevicePortForwarder.test({
+    @required ProcessManager processManager,
+    @required Logger logger,
+    String id,
+  }) {
+    return IOSDevicePortForwarder(
+      processManager: processManager,
+      logger: logger,
+      iproxyPath: 'iproxy',
+      id: id ?? '1234',
+      dyLdLibEntry: const MapEntry<String, String>(
+        'DYLD_LIBRARY_PATH', '/path/to/libs',
+      ),
+    );
+  }
+
+  final ProcessUtils _processUtils;
+  final Logger _logger;
+  final MapEntry<String, String> _dyLdLibEntry;
+  final String _id;
+  final String _iproxyPath;

  @override
-  List<ForwardedPort> get forwardedPorts => _forwardedPorts;
+  List<ForwardedPort> forwardedPorts = <ForwardedPort>[];

  @visibleForTesting
-  void addForwardedPorts(List<ForwardedPort> forwardedPorts) {
-    forwardedPorts.forEach(_forwardedPorts.add);
+  void addForwardedPorts(List<ForwardedPort> ports) {
+    ports.forEach(forwardedPorts.add);
  }

  static const Duration _kiProxyPortForwardTimeout = Duration(seconds: 1);
@@ -612,17 +655,17 @@ class IOSDevicePortForwarder extends DevicePortForwarder {

    bool connected = false;
    while (!connected) {
-      globals.printTrace('Attempting to forward device port $devicePort to host port $hostPort');
+      _logger.printTrace('Attempting to forward device port $devicePort to host port $hostPort');
      // Usage: iproxy LOCAL_TCP_PORT DEVICE_TCP_PORT UDID
-      process = await processUtils.start(
+      process = await _processUtils.start(
        <String>[
-          device._iproxyPath,
+          _iproxyPath,
          hostPort.toString(),
          devicePort.toString(),
-          device.id,
+          _id,
        ],
        environment: Map<String, String>.fromEntries(
-          <MapEntry<String, String>>[globals.cache.dyLdLibEntry],
+          <MapEntry<String, String>>[_dyLdLibEntry],
        ),
      );
      // TODO(ianh): This is a flakey race condition, https://github.com/libimobiledevice/libimobiledevice/issues/674
@@ -645,25 +688,25 @@ class IOSDevicePortForwarder extends DevicePortForwarder {
    final ForwardedPort forwardedPort = ForwardedPort.withContext(
      hostPort, devicePort, process,
    );
-    globals.printTrace('Forwarded port $forwardedPort');
-    _forwardedPorts.add(forwardedPort);
+    _logger.printTrace('Forwarded port $forwardedPort');
+    forwardedPorts.add(forwardedPort);
    return hostPort;
  }

  @override
  Future<void> unforward(ForwardedPort forwardedPort) async {
-    if (!_forwardedPorts.remove(forwardedPort)) {
+    if (!forwardedPorts.remove(forwardedPort)) {
      // Not in list. Nothing to remove.
      return;
    }

-    globals.printTrace('Unforwarding port $forwardedPort');
+    _logger.printTrace('Unforwarding port $forwardedPort');
    forwardedPort.dispose();
  }

  @override
  Future<void> dispose() async {
-    for (final ForwardedPort forwardedPort in _forwardedPorts) {
+    for (final ForwardedPort forwardedPort in forwardedPorts) {
      forwardedPort.dispose();
    }
  }
--- a/packages/flutter_tools/test/general.shard/ios/devices_test.dart
+++ b/packages/flutter_tools/test/general.shard/ios/devices_test.dart
@@ -248,7 +248,13 @@ void main() {
      IOSDevicePortForwarder createPortForwarder(
          ForwardedPort forwardedPort,
          IOSDevice device) {
-        final IOSDevicePortForwarder portForwarder = IOSDevicePortForwarder(device);
+        final IOSDevicePortForwarder portForwarder = IOSDevicePortForwarder(
+          dyLdLibEntry: mockCache.dyLdLibEntry,
+          id: device.id,
+          iproxyPath: mockArtifacts.getArtifactPath(Artifact.iproxy, platform: TargetPlatform.ios),
+          logger: logger,
+          processManager: FakeProcessManager.any(),
+        );
        portForwarder.addForwardedPorts(<ForwardedPort>[forwardedPort]);
        return portForwarder;
      }
@@ -482,54 +488,6 @@ void main() {
        Usage: () => mockUsage,
      });

-      // By default, the .forward() method will try every port between 1024
-      // and 65535; this test verifies we are killing iproxy processes when
-      // we timeout on a port
-      testUsingContext('.forward() will kill iproxy processes before invoking a second', () async {
-        const String deviceId = '123';
-        const int devicePort = 456;
-        final IOSDevice device = IOSDevice(
-          deviceId,
-          artifacts: mockArtifacts,
-          fileSystem: mockFileSystem,
-          platform: macPlatform,
-          iosDeploy: iosDeploy,
-          name: 'iPhone 1',
-          sdkVersion: '13.3',
-          cpuArchitecture: DarwinArch.arm64,
-        );
-        final IOSDevicePortForwarder portForwarder = IOSDevicePortForwarder(device);
-        bool firstRun = true;
-        final MockProcess successProcess = MockProcess(
-          exitCode: Future<int>.value(0),
-          stdout: Stream<List<int>>.fromIterable(<List<int>>['Hello'.codeUnits]),
-        );
-        final MockProcess failProcess = MockProcess(
-          exitCode: Future<int>.value(1),
-          stdout: const Stream<List<int>>.empty(),
-        );
-
-        final ProcessFactory factory = (List<String> command) {
-          if (!firstRun) {
-            return successProcess;
-          }
-          firstRun = false;
-          return failProcess;
-        };
-        mockProcessManager.processFactory = factory;
-        final int hostPort = await portForwarder.forward(devicePort);
-        // First port tried (1024) should fail, then succeed on the next
-        expect(hostPort, 1024 + 1);
-        verifyNever(successProcess.kill());
-        verify(failProcess.kill());
-      }, overrides: <Type, Generator>{
-        Artifacts: () => mockArtifacts,
-        Cache: () => mockCache,
-        Platform: () => macPlatform,
-        ProcessManager: () => mockProcessManager,
-        Usage: () => mockUsage,
-      });
-
      testUsingContext('succeeds in debug mode when mDNS fails by falling back to manual protocol discovery', () async {
        final IOSDevice device = IOSDevice(
          '123',
--- a/packages/flutter_tools/test/general.shard/ios/ios_device_port_forwarder_test.dart
+++ b/packages/flutter_tools/test/general.shard/ios/ios_device_port_forwarder_test.dart
@@ -0,0 +1,46 @@
+// 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:flutter_tools/src/base/logger.dart';
+import 'package:flutter_tools/src/ios/devices.dart';
+
+import '../../src/common.dart';
+import '../../src/context.dart';
+
+const Map<String, String> kDyLdLibEntry = <String, String>{
+  'DYLD_LIBRARY_PATH': '/path/to/libs',
+};
+
+void main() {
+  // By default, the .forward() method will try every port between 1024
+  // and 65535; this test verifies we are killing iproxy processes when
+  // we timeout on a port
+  testWithoutContext('IOSDevicePortForwarder.forward will kill iproxy processes before invoking a second', () async {
+    const int devicePort = 456;
+    final FakeProcessManager processManager = FakeProcessManager.list(<FakeCommand>[
+      const FakeCommand(
+        command: <String>['iproxy', '1024', '456', '1234'],
+        // iproxy does not exit with 0 when it cannot forward.
+        exitCode: 0,
+        stdout: null, // no stdout indicates failure.
+        environment: kDyLdLibEntry,
+      ),
+      const FakeCommand(
+        command: <String>['iproxy', '1025', '456', '1234'],
+        exitCode: 0,
+        stdout: 'not empty',
+        environment: kDyLdLibEntry,
+      ),
+    ]);
+    final IOSDevicePortForwarder portForwarder = IOSDevicePortForwarder.test(
+      processManager: processManager,
+      logger: BufferLogger.test(),
+    );
+    final int hostPort = await portForwarder.forward(devicePort);
+
+    // First port tried (1024) should fail, then succeed on the next
+    expect(hostPort, 1024 + 1);
+    expect(processManager.hasRemainingExpectations, false);
+  });
+}
--- a/packages/flutter_tools/test/src/fake_process_manager.dart
+++ b/packages/flutter_tools/test/src/fake_process_manager.dart
@@ -30,9 +30,7 @@ class FakeCommand {
    this.completer,
  }) : assert(command != null),
       assert(duration != null),
-       assert(exitCode != null),
-       assert(stdout != null),
-       assert(stderr != null);
+       assert(exitCode != null);

  /// The exact commands that must be matched for this [FakeCommand] to be
  /// considered correct.
@@ -139,8 +137,12 @@ class _FakeProcess implements Process {
        }
        return _exitCode;
      }),
-      stderr = Stream<List<int>>.value(utf8.encode(_stderr)),
-      stdout = Stream<List<int>>.value(utf8.encode(_stdout));
+      stderr = _stderr == null
+        ? const Stream<List<int>>.empty()
+        : Stream<List<int>>.value(utf8.encode(_stderr)),
+      stdout = _stdout == null
+        ? const Stream<List<int>>.empty()
+        : Stream<List<int>>.value(utf8.encode(_stdout));

  final int _exitCode;