-
-
Notifications
You must be signed in to change notification settings - Fork 424
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
fix xctestrun file detection when useXctestrunFile is true #903
Conversation
@@ -274,7 +275,8 @@ class XCUITestDriver extends BaseDriver { | |||
log.info(`Xcode version set to '${this.xcodeVersion.versionString}' ${tools}`); | |||
|
|||
this.iosSdkVersion = await getAndCheckIosSdkVersion(); | |||
log.info(`iOS SDK Version set to '${this.iosSdkVersion}'`); | |||
this.opts.iosSdkVersion = this.iosSdkVersion; // Pass to xcodebuild |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.iosSdkVersion
is shared with ios-driver. Thus, I've added it as a part of opts
since the opts
is an argument of WDA.
README.md
Outdated
@@ -148,7 +148,7 @@ Differences noted here | |||
|`calendarAccessAuthorized`|Set this to `true` if you want to enable calendar access on IOS Simulator with given bundleId. Set to `false`, if you want to disable calendar access on IOS Simulator with given bundleId. If not set, the calendar authorization status will not be set.|e.g., `true`| | |||
|`isHeadless`|Set this capability to `true` if automated tests are running on Simulator and the device display is not needed to be visible. This only has an effect since Xcode9 and only for simulators. All running instances of Simulator UI are going to be automatically terminated if headless test is started. `false` is the default value.|e.g., `true`| | |||
|`webkitDebugProxyPort`|Local port number used for communication with ios-webkit-debug-proxy. Only relevant for real devices. The default value equals to `27753`.|e.g. `20000`| | |||
|`useXctestrunFile`|Use Xctestrun file to launch WDA. It will search for such file in `bootstrapPath`. Expected name of file is `WebDriverAgentRunner_iphoneos<platformVersion>-arm64.xctestrun` for real device and `WebDriverAgentRunner_iphonesimulator<platformVersion>-x86_64.xctestrun` for simulator. One can do `build-for-testing` for `WebDriverAgent` project for simulator and real device and then you will see [Product Folder like this](docs/useXctestrunFile.png) and you need to copy content of this folder at `bootstrapPath` location. Since, this capability expects that you have already built `WDA` project, it neither check whether you have necessary dependencies to build `WDA` nor it try to build project. Defaults to `false`|e.g., `true`| | |||
|`useXctestrunFile`|Use Xctestrun file to launch WDA. It will search for such file in `bootstrapPath`. Expected name of file is `WebDriverAgentRunner_iphoneos<sdkVersion>-arm64.xctestrun` for real device and `WebDriverAgentRunner_iphonesimulator<sdkVersion>-x86_64.xctestrun` for simulator. One can do `build-for-testing` for `WebDriverAgent` project for simulator and real device and then you will see [Product Folder like this](docs/useXctestrunFile.png) and you need to copy content of this folder at `bootstrapPath` location. Since, this capability expects that you have already built `WDA` project, it neither check whether you have necessary dependencies to build `WDA` nor it try to build project. Defaults to `false`|e.g., `true`| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this capability expects that you have already built WDA
project, it neither checks whether you have necessary dependencies to build WDA
nor will it try to build project.
@@ -274,7 +275,8 @@ class XCUITestDriver extends BaseDriver { | |||
log.info(`Xcode version set to '${this.xcodeVersion.versionString}' ${tools}`); | |||
|
|||
this.iosSdkVersion = await getAndCheckIosSdkVersion(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use this.opts.iosSdkVersion
and do away with this.iosSdkVersion
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to
appium-xcuitest-driver/lib/driver.js
Lines 165 to 176 in 6e6eefd
// some things that commands imported from appium-ios-driver need | |
this.curWebFrames = []; | |
this.webElementIds = []; | |
this._currentUrl = null; | |
this.curContext = null; | |
this.xcodeVersion = {}; | |
this.iosSdkVersion = null; | |
this.contexts = []; | |
this.implicitWaitMs = 0; | |
this.asynclibWaitMs = 0; | |
this.pageLoadMs = 6000; | |
this.landscapeWebCoordsOffset = 0; |
this.iosSdkVersion
is imported from ios-driver. thus, I added this.opts.iosSdkVersion
keeping this.iosSdkVersion
to avoid affection to this.iosSdkVersion.
I replaced this.iosSdkVersion to this.opts.iosSdkVersion once and ran some tests without issues, but I thought the change should have created as another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with that.
lib/wda/utils.js
Outdated
* @param {string} sdkVersion - The Xcode SDK version of OS. | ||
* @param {string} bootstrapPath - The folder path containing xctestrun file. | ||
*/ | ||
async function getXctestrunFilePath (isRealDevice, udid, platformVersion, sdkVersion, bootstrapPath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of putting all the declarations at the beginning, even when they won't be used for a long time.
How about something like:
async function getXctestrunFilePath (isRealDevice, udid, platformVersion, sdkVersion, bootstrapPath) {
// First try the SDK path, for Xcode 10 (at least)
const sdkBased = [
path.resolve(bootstrapPath, `${udid}_${sdkVersion}.xctestrun`),
sdkVersion,
];
// Next try Platform path, for earlier Xcode versions
const platformBased = [
path.resolve(bootstrapPath, `${udid}_${platformVersion}.xctestrun`),
platformVersion,
];
for (const [filePath, version] of _.toPairs([sdkBased, platformBased])) {
if (await fs.exists(filePath)) {
return filePath;
}
const originalXctestrunFile = path.resolve(bootstrapPath, _getXctestrunFilePathName(isRealDevice, version));
if (await fs.exists(originalXctestrunFile)) {
// If this is first time run for given device, then first generate xctestrun file for device.
// We need to have a xctestrun file **per device** because we cant not have same wda port for all devices.
await fs.copyFile(originalXctestrunFile, filePath);
return filePath;
}
}
log.errorAndThrow(`If you are using 'useXctestrunFile' capability then you ` +
`need to have an xctestrun file (expected: ` +
`'${path.resolve(bootstrapPath, _getXctestrunFilePathName(isRealDevice, sdkVersion))}')`);
}
lib/wda/utils.js
Outdated
} | ||
|
||
function _getXctestrunFilePathName (isRealDevice, version) { | ||
return isRealDevice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return `WebDriverAgentRunner_iphone${isRealDevice ? 'os' : 'simulator'}${version}-arm64.xctestrun`
lib/wda/utils.js
Outdated
path.resolve(bootstrapPath, _getXctestrunFilePathName(isRealDevice, sdkVersion))} file`); | ||
} | ||
|
||
function _getXctestrunFilePathName (isRealDevice, version) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the underscore prefix?
lib/wda/utils.js
Outdated
* @param {string} sdkVersion - The Xcode SDK version of OS. | ||
* @param {string} bootstrapPath - The folder path containing xctestrun file. | ||
*/ | ||
async function getXctestrunFilePath (isRealDevice, udid, platformVersion, sdkVersion, bootstrapPath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like functions with 3 or more arguments. What if we wrap isRealDevice, udid, platformVersion
into a single object, because they all are actually properties of the particular device under test?
* fix xctestrun path * tweak handking sdk version * tuen a bit * update caps in readme * add a test to raise an error * move control flow into string * fix reviews * add a tips for building module
When
useXctestrunFile
is true, Appium refersWebDriverAgentRunner_iphoneos${platformVersion}-arm64.xctestrun
.But I noticed recent Xcode (9.4, 10.1 and 10.2 at least) generates them based on sdk versions like
WebDriverAgentRunner_iphoneos${sdkVersion}-arm64.xctestrun
.I'm not sure from which Xcode version starts generating them following sdk versions.
So, in this PR, I changed as:
xctestrun
following sdk versionFrom this PR, we can resolve
xctestrun
path with only iOS SDK version x Real/Simulator combination.(No need to consider platformVersion x Real/Simulator combination)
e.g.
we can re-use
xctestrun
generated by Xcode 10.2 (SDK 12.2) for iPhone 11.4, 12.0, 12.2 etc.When we run tests in 3 parallel, xctestrun will be copied as below