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

feat: bump generic iPhone for iOS 13.0 #1068

Merged
merged 1 commit into from
Sep 16, 2019
Merged

feat: bump generic iPhone for iOS 13.0 #1068

merged 1 commit into from
Sep 16, 2019

Conversation

dpgraham
Copy link
Contributor

What this pull request does

  • Bumps up the default device for the generic iphone simulator for iOS 13+ from iPhone 6 to iPhone X

WHY?: iOS 13 does not support iPhone 6 so this needs to be changed. Chose iPhone X over iPhone 11 because it's not quite the latest and not bleeding edge.

  • Refactored logic so that the default generic versions are specified in a plain Javascript object in ios-generic-simulators.js

WHY?: So we're not hardcoding this in code. We just need to keep this file (and the tests) up-to-date

@triager triager force-pushed the dpgraham-bump-default branch from 4da3461 to d7a56bc Compare September 16, 2019 17:42
mykola-mokhnach
mykola-mokhnach reviewed Sep 16, 2019
View reviewed changes
lib/utils.js Outdated
let genericIosSimulator;

// Find the highest iOS version in the list that is below the provided version
for (const [platformVersionFromList, iosSimulator] of genericSimulators) {
Copy link
Contributor

Choose a reason for hiding this comment

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

_.toPairs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They aren't pairs. It's a list of lists

"iPhone": [
  [<ios-version>, <default>],
  [<ios-version-next>, <default-next>],
  ...
],

mykola-mokhnach
mykola-mokhnach reviewed Sep 16, 2019
View reviewed changes
lib/utils.js Outdated
* @param {string} deviceName Type of IOS device. Can be iPhone, iPad (possibly more in the future)
*/
function getGenericSimulatorForIosVersion (platformVersion, deviceName) {
const genericSimulators = iosGenericSimulators[deviceName];
Copy link
Contributor

Choose a reason for hiding this comment

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

what if this returns undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just fixed that now. That did cause an error.

mykola-mokhnach
mykola-mokhnach reviewed Sep 16, 2019
View reviewed changes
lib/utils.js Outdated
if (deviceName !== devName) {
log.debug(`Changing deviceName from '${devName}' to '${deviceName}'`);
function translateDeviceName (platformVersion, deviceName = '') {
const translatedDeviceName = getGenericSimulatorForIosVersion(platformVersion, deviceName.toLowerCase().trim());
Copy link
Contributor

Choose a reason for hiding this comment

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

what if this is undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't be now. The getGenericSimulatorForIosVersion always returns device name.

imurchie
imurchie reviewed Sep 16, 2019
View reviewed changes
Copy link
Contributor

@imurchie imurchie left a comment

Choose a reason for hiding this comment

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

Good idea!

lib/ios-generic-simulators.js
@@ -0,0 +1,10 @@
export default {
'ipad simulator': [
['0.0', 'iPad Retina'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth noting that these need to be strictly in order, or the parsing logic won't work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Maybe I should just sort them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I sort them now to be safe.

lib/utils.js Outdated
if (deviceName !== devName) {
log.debug(`Changing deviceName from '${devName}' to '${deviceName}'`);
function translateDeviceName (platformVersion, deviceName = '') {
const translatedDeviceName = getGenericSimulatorForIosVersion(platformVersion, deviceName.toLowerCase().trim());
Copy link
Contributor

Choose a reason for hiding this comment

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

I would opt for a variable name that is not so close to the function name, for readability's sake.

@triager triager force-pushed the dpgraham-bump-default branch 2 times, most recently from 305ffe6 to 37cc52b Compare September 16, 2019 18:21
mykola-mokhnach
mykola-mokhnach reviewed Sep 16, 2019
View reviewed changes
lib/utils.js
* Get the generic simulator for a given IOS version and device type (iPhone, iPad)
*
* @param {string|number} platformVersion IOS version. e.g.) 13.0
* @param {string} deviceName Type of IOS device. Can be iPhone, iPad (possibly more in the future)
Copy link
Contributor

Choose a reason for hiding this comment

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

missing return description

mykola-mokhnach
mykola-mokhnach reviewed Sep 16, 2019
View reviewed changes
lib/utils.js Outdated
function getGenericSimulatorForIosVersion (platformVersion, deviceName) {
let genericSimulators = iosGenericSimulators[deviceName];

//console.log(util.compareVersions('0.0', '>', '10.3')); process.exit();
Copy link
Contributor

Choose a reason for hiding this comment

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

is this comment needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@triager triager force-pushed the dpgraham-bump-default branch from 37cc52b to 3d29855 Compare September 16, 2019 18:29
mykola-mokhnach
mykola-mokhnach reviewed Sep 16, 2019
View reviewed changes
lib/utils.js
}
genericIosSimulator = iosSimulator;
}
return genericIosSimulator;
Copy link
Contributor

Choose a reason for hiding this comment

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

can the result be undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. The lowest iOS version in the list is 0.0 so every iOS version we test it against will be above that.

Copy link
Contributor

Choose a reason for hiding this comment

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

it makes sense to leave a comment about that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it so now it can be undefined and translateDeviceName handles that case.

mykola-mokhnach
mykola-mokhnach reviewed Sep 16, 2019
View reviewed changes
lib/utils.js
return deviceName;
}

function translateDeviceName (platformVersion, deviceName = '') {
const deviceNameTranslated = getGenericSimulatorForIosVersion(platformVersion, deviceName.toLowerCase().trim());
Copy link
Contributor

Choose a reason for hiding this comment

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

is it necessary to change the device name to lower case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mykola-mokhnach yeah if somebody provides iPhone Simulator we want it to be case-insensitive.

@triager triager force-pushed the dpgraham-bump-default branch 2 times, most recently from bc62b9c to f5f87f7 Compare September 16, 2019 20:01
@dpgraham
Copy link
Contributor Author

@mykola-mokhnach @imurchie fixes pushed.

@triager triager force-pushed the dpgraham-bump-default branch from f5f87f7 to 52116c4 Compare September 16, 2019 20:05
@dpgraham dpgraham merged commit 649f4fe into master Sep 16, 2019
@dpgraham dpgraham deleted the dpgraham-bump-default branch September 16, 2019 21:57
khanayan123 pushed a commit to khanayan123/appium-xcuitest-driver that referenced this pull request May 10, 2021
@dpgraham
feat: bump generic iPhone for iOS 13.0 (appium#1068)
afa7e2e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mykola-mokhnach mykola-mokhnach mykola-mokhnach left review comments

@imurchie imurchie imurchie left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dpgraham @imurchie @mykola-mokhnach