Skip to content

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

Merged
jonahwilliams merged 3 commits intoflutter:masterfrom
jonahwilliams:device_port_forwarder_fixes
Mar 18, 2020
Merged

[flutter_tools] refactor the IOSDevicePortForwarder and move tests out of devices_test.dart#52772
jonahwilliams merged 3 commits intoflutter:masterfrom
jonahwilliams:device_port_forwarder_fixes

Conversation

@jonahwilliams
Copy link
Copy Markdown
Contributor

Description

(Note: I tried the lib only changes, but those need test changes too .. so it got pretty big. Trying vertical cuts instead of horizontal).

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.

@fluttergithubbot fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 17, 2020

@override
DevicePortForwarder get portForwarder => _portForwarder ??= IOSDevicePortForwarder(this);
DevicePortForwarder get portForwarder => _portForwarder ??= IOSDevicePortForwarder(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not referenced in google3


@override
List<ForwardedPort> get forwardedPorts => _forwardedPorts;
List<ForwardedPort> forwardedPorts = <ForwardedPort>[];
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is semantically equivalent, just shorter

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a valid override for List<ForwardedPort> get forwardedPorts? TIL.

Copy link
Copy Markdown
Contributor Author

@jonahwilliams jonahwilliams Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dart doesn't actually have fields, so:

int get a => _a;
set a(int value) => _a = value;

is equivalent to

int a;

And

int get a => _a;

Is equivalent to a final field like above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(edited, code typo)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL

final List<ForwardedPort> _forwardedPorts;
final ProcessUtils _processUtils;
final Logger _logger;
final MapEntry<String, String> _dyLdLibEntry;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-trivial change: I inject the dyLdLibEntry and iproxy paths.

}),
stderr = Stream<List<int>>.value(utf8.encode(_stderr)),
stdout = Stream<List<int>>.value(utf8.encode(_stdout));
stderr = _stderr == null
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We needed a way to differentiate the stream being empty versus containing an empty string.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does utf8.encode('') return?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EmptyList, but the Stream constructor leads to a non-empty stream with a single empty list emitted on it.

// 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 {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only have a single IOSDevicePortForwarder test :(

Copy link
Copy Markdown
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jonahwilliams jonahwilliams merged commit 90d6169 into flutter:master Mar 18, 2020
@jonahwilliams jonahwilliams deleted the device_port_forwarder_fixes branch March 18, 2020 00:58
@lock
Copy link
Copy Markdown

lock Bot commented Apr 2, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@lock lock Bot locked and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants