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.

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

refactor: simplify createDevice logic #89

Merged
merged 9 commits into from
Nov 20, 2019
Merged

refactor: simplify createDevice logic #89

merged 9 commits into from
Nov 20, 2019

Conversation

imurchie
Copy link
Contributor

Make sure that we can create simulators when the platform version and runtimes are different (which is mostly to say, when there is a patch version of a sim)

So, if xcrun simctl list runtimes returns:

== Runtimes ==
iOS 9.3 (9.3 - 13E233) - com.apple.CoreSimulator.SimRuntime.iOS-9-3
iOS 10.3 (10.3.1 - 14E8301) - com.apple.CoreSimulator.SimRuntime.iOS-10-3
iOS 11.0 (11.0.1 - 15A8401) - com.apple.CoreSimulator.SimRuntime.iOS-11-0
iOS 11.1 (11.1 - 15B87) - com.apple.CoreSimulator.SimRuntime.iOS-11-1
iOS 11.2 (11.2 - 15C107) - com.apple.CoreSimulator.SimRuntime.iOS-11-2
iOS 11.3 (11.3 - 15E217) - com.apple.CoreSimulator.SimRuntime.iOS-11-3
iOS 11.4 (11.4 - 15F79) - com.apple.CoreSimulator.SimRuntime.iOS-11-4
iOS 12.0 (12.0 - 16A366) - com.apple.CoreSimulator.SimRuntime.iOS-12-0
iOS 12.1 (12.1 - 16B91) - com.apple.CoreSimulator.SimRuntime.iOS-12-1
iOS 12.2 (12.2 - 16E226) - com.apple.CoreSimulator.SimRuntime.iOS-12-2
iOS 12.4 (12.4 - 16G73) - com.apple.CoreSimulator.SimRuntime.iOS-12-4
iOS 13.0 (13.0 - 17A577) - com.apple.CoreSimulator.SimRuntime.iOS-13-0
iOS 13.1 (13.1 - 17A844) - com.apple.CoreSimulator.SimRuntime.iOS-13-1
iOS 13.2 (13.2.2 - 17B102) - com.apple.CoreSimulator.SimRuntime.iOS-13-2
tvOS 13.2 (13.2 - 17K90) - com.apple.CoreSimulator.SimRuntime.tvOS-13-2
watchOS 6.1 (6.1 - 17S83) - com.apple.CoreSimulator.SimRuntime.watchOS-6-1

And someone wants iOS 13.2 it will currently fail because the runtime will be com.apple.CoreSimulator.SimRuntime.iOS-13-2-2 rather than the correct com.apple.CoreSimulator.SimRuntime.iOS-13-2. In this PR it just tries out the various combinations, in order to find the correct one.

In the case of Xcode 11.2.1, requesting iOS 13.2, will produce array of possible runtime ids:

[ 
  'com.apple.CoreSimulator.SimRuntime.iOS-13-2',
  'com.apple.CoreSimulator.SimRuntime.iOS-13-2-2',
  '13.2',
  '13.2.2'
 ]

if (isNewerIdFormatRequired) {
runtimeId = `${SIM_RUNTIME_NAME}${platform}-${runtimeId.replace(/\./g, '-')}`;
let potentialRuntimeIds = [`${runtimeIdSemver.major}.${runtimeIdSemver.minor}`];
if (runtimeId.split('.').length === 3) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (runtimeIdSemver.patch) ...

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'm not sure that there is no situation where a 0 patch version is not valid, and it would be excluded here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with the current implementation as well

lib/simctl.js Outdated
break;
} catch (err) {
const reason = err.stderr ? err.stderr.trim() : err.message;
log.warn(`Unable to create simulator: ${reason}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to print these warnings? we anyway know some of these ids won't work

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it only makes sense to print them if the whole sim creation op fails

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 think it would be confusing if it were not there, as there would be multiple lines saying it was creating a simulator, with no indication of why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Played around a bit and the error gets logged elsewhere, so the confusion I envisioned is not there.

lib/simctl.js Outdated
if (runtimeId.split('.').length === 3) {
potentialRuntimeIds.push(runtimeId);
}
for (const id of potentialRuntimeIds) {
Copy link
Contributor

@mykola-mokhnach mykola-mokhnach Nov 18, 2019

Choose a reason for hiding this comment

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

runtimeIds.push(
  ...(potentialRuntimeIds.map((id) => `${SIM_RUNTIME_NAME}${platform}-${id.replace(/\./g, '-')}`)),
  ...potentialRuntimeIds
)

lib/simctl.js Outdated
}
}
}
const done = _.values(await getDevices())
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

lib/simctl.js Outdated
.filter((device) => device.udid === udid)
.every((device) => device.state !== 'Creating');
if (!done) {
throw new Error('Device still being created');
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also print the device name?

@mykola-mokhnach
Copy link
Contributor

It looks like tests need some love

@imurchie
Copy link
Contributor Author

you could use .some

What is this referring to?

@mykola-mokhnach
Copy link
Contributor

you could use .some

What is this referring to?

https://github.com/appium/node-simctl/pull/89/files#diff-a020ec8175f353e84bb9b28d0a2b31c0R428

@imurchie imurchie merged commit 96c5ce6 into master Nov 20, 2019
@imurchie imurchie deleted the isaac-create branch November 20, 2019 16:25
@imurchie imurchie mentioned this pull request Dec 11, 2019
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants