Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prevent crashes on range errors when selecting device #129290

Merged
merged 7 commits into from Jun 22, 2023
Merged

Prevent crashes on range errors when selecting device #129290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b6b6094
Prevent user from typing 0 during device selection
eliasyishak Jun 21, 2023
472c8fe
Add test + correct selection logic for `TargetDevices`
eliasyishak Jun 21, 2023
5326296
Revert unnecessary logic for `TargetDevices`
eliasyishak Jun 21, 2023
931565d
Removing accompanying test for `TargetDevices`
eliasyishak Jun 21, 2023
530f8a9
Clean up
eliasyishak Jun 21, 2023
dc1315a
Adding test to validate having invalid user inputs for device
eliasyishak Jun 21, 2023
2091fcd
Add device id that exceeds top bounds
eliasyishak Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/flutter_tools/lib/src/runner/target_devices.dart
View file
Open in desktop
Expand Up @@ -774,7 +774,7 @@ class TargetDeviceSelection {
throwToolExit('');
}
final int deviceIndex = int.parse(userInputString) - 1;
if (deviceIndex < devices.length) {
if (deviceIndex > -1 && deviceIndex < devices.length) {
chosenDevice = devices[deviceIndex];
}
}
Expand Down
73 changes: 63 additions & 10 deletions packages/flutter_tools/test/general.shard/runner/target_devices_test.dart
View file
Open in desktop
Expand Up @@ -1459,7 +1459,7 @@ No devices found yet. Checking for wireless devices...
logger: logger,
);
targetDevices.waitForWirelessBeforeInput = true;
targetDevices.deviceSelection.input = '1';
targetDevices.deviceSelection.input = <String>['1'];
logger.originalStatusText = '''
Connected devices:
target-device-9 (mobile) • xxx • ios • iOS 16
Expand All @@ -1486,6 +1486,49 @@ Please choose one (or "q" to quit): '''));
}, overrides: <Type, Generator>{
AnsiTerminal: () => terminal,
});


testUsingContext('handle invalid options for device', () async {
deviceManager.iosDiscoverer.deviceList = <Device>[nonEphemeralDevice];

final TestTargetDevicesWithExtendedWirelessDeviceDiscovery targetDevices = TestTargetDevicesWithExtendedWirelessDeviceDiscovery(
deviceManager: deviceManager,
logger: logger,
);
targetDevices.waitForWirelessBeforeInput = true;

// Having the '0' first is an invalid choice for a device, the second
// item in the list is a '2' which is out of range since we only have
// one item in the deviceList. The final item in the list, is '1'
// which is a valid option though which will return a valid device
//
// Important: if none of the values in the list are valid, the test will
// hang indefinitely since the [userSelectDevice()] method uses a while
// loop to listen for valid devices
targetDevices.deviceSelection.input = <String>['0', '2', '1'];
logger.originalStatusText = '''
Connected devices:
target-device-9 (mobile) • xxx • ios • iOS 16

Checking for wireless devices...

[1]: target-device-9 (xxx)
''';

final List<Device>? devices = await targetDevices.findAllTargetDevices();

expect(logger.statusText, equals('''
Connected devices:
target-device-9 (mobile) • xxx • ios • iOS 16

No wireless devices were found.

[1]: target-device-9 (xxx)
Please choose one (or "q" to quit): '''));
expect(devices, <Device>[nonEphemeralDevice]);
}, overrides: <Type, Generator>{
AnsiTerminal: () => terminal,
});
});

group('without stdinHasTerminal', () {
Expand Down Expand Up @@ -1753,7 +1796,7 @@ Checking for wireless devices...
];

targetDevices.waitForWirelessBeforeInput = true;
targetDevices.deviceSelection.input = '3';
targetDevices.deviceSelection.input = <String>['3'];
logger.originalStatusText = '''
Connected devices:
target-device-1 (mobile) • xxx • ios • iOS 16
Expand Down Expand Up @@ -1791,7 +1834,7 @@ Please choose one (or "q" to quit): '''));
deviceManager.iosDiscoverer.deviceList = <Device>[attachedIOSDevice1, attachedIOSDevice2];

targetDevices.waitForWirelessBeforeInput = true;
targetDevices.deviceSelection.input = '2';
targetDevices.deviceSelection.input = <String>['2'];
logger.originalStatusText = '''
Connected devices:
target-device-1 (mobile) • xxx • ios • iOS 16
Expand Down Expand Up @@ -1828,7 +1871,7 @@ Please choose one (or "q" to quit): '''));
deviceManager.iosDiscoverer.refreshDeviceList = <Device>[connectedWirelessIOSDevice1, connectedWirelessIOSDevice2];

targetDevices.waitForWirelessBeforeInput = true;
targetDevices.deviceSelection.input = '2';
targetDevices.deviceSelection.input = <String>['2'];
terminal.setPrompt(<String>['1', '2', 'q', 'Q'], '1');

final List<Device>? devices = await targetDevices.findAllTargetDevices();
Expand Down Expand Up @@ -1906,7 +1949,7 @@ target-device-5 (mobile) • xxx • ios • iOS 16
deviceManager.iosDiscoverer.deviceList = <Device>[attachedIOSDevice1, attachedIOSDevice2];

targetDevices.waitForWirelessBeforeInput = true;
targetDevices.deviceSelection.input = '2';
targetDevices.deviceSelection.input = <String>['2'];
logger.originalStatusText = '''
Connected devices:
target-device-1 (mobile) • xxx • ios • iOS 16
Expand Down Expand Up @@ -1951,7 +1994,7 @@ Please choose one (or "q" to quit): '''));
deviceManager.iosDiscoverer.refreshDeviceList = <Device>[attachedIOSDevice1, attachedIOSDevice2, connectedWirelessIOSDevice1];

targetDevices.waitForWirelessBeforeInput = true;
targetDevices.deviceSelection.input = '2';
targetDevices.deviceSelection.input = <String>['2'];
logger.originalStatusText = '''
Connected devices:
target-device-1 (mobile) • xxx • ios • iOS 16
Expand Down Expand Up @@ -2133,7 +2176,7 @@ target-device-6 (mobile) • xxx • ios • iOS 16
];

targetDevices.waitForWirelessBeforeInput = true;
targetDevices.deviceSelection.input = '3';
targetDevices.deviceSelection.input = <String>['3'];
logger.originalStatusText = '''
Found multiple devices with name or id matching target-device:
target-device-1 (mobile) • xxx • ios • iOS 16
Expand Down Expand Up @@ -2173,7 +2216,7 @@ Please choose one (or "q" to quit): '''));
deviceManager.iosDiscoverer.deviceList = <Device>[attachedIOSDevice1, attachedIOSDevice2];

targetDevices.waitForWirelessBeforeInput = true;
targetDevices.deviceSelection.input = '2';
targetDevices.deviceSelection.input = <String>['2'];
logger.originalStatusText = '''
Found multiple devices with name or id matching target-device:
target-device-1 (mobile) • xxx • ios • iOS 16
Expand Down Expand Up @@ -2443,11 +2486,21 @@ class TestTargetDevicesWithExtendedWirelessDeviceDiscovery extends TargetDevices
class TestTargetDeviceSelection extends TargetDeviceSelection {
TestTargetDeviceSelection(super.logger);

String input = '';
List<String> input = <String>[];

@override
Future<String> readUserInput() async {
return input;
// If only one value is provided for the input, continue
// to return that one input value without popping
//
// If more than one input values are provided, we are simulating
// the user selecting more than one option for a device, so we will pop
// them out from the front
if (input.length > 1) {
return input.removeAt(0);
}

return input[0];
Comment on lines +2489 to +2503
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @vashworth, i reworked this method so that we provide lists of inputs for selecting a device.

Thoughts? The while loop in the TargetDeviceSelection.userSelectDevice() method makes it a bit difficult to mock multiple inputs over a test

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense to me!

}
}

Expand Down