fix: uninstall installed uia2 servers if they were greater than on-going session #796
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lib/uiautomator2.js
Outdated
| * @param {PackageInfo[]} packagesInfo | ||
| * @returns {boolean} | ||
| */ | ||
| canInstallServerPackages (packagesInfo = []) { |
There was a problem hiding this comment.
it's rather shouldInstall
lib/uiautomator2.js
Outdated
| * @returns {boolean} true if any of components is not installed or older than currently installed in order to | ||
| * install or upgrade the servers on the device under test. | ||
| */ | ||
| shouldInstall(packagesInfo = []) { |
There was a problem hiding this comment.
shouldInstallServerPackages
lib/uiautomator2.js
Outdated
| const shouldUninstallServerPackages = (packagesInfo.some(({installState}) => installState === this.adb.APP_INSTALL_STATE.NOT_INSTALLED) | ||
| && !packagesInfo.every(({installState}) => installState === this.adb.APP_INSTALL_STATE.NOT_INSTALLED)); | ||
| // To ensure if the UIA2 driver should uninstall UI2 server packages from the device | ||
| // to use peoper UIA2 server versions current UIA2 server wants to use. |
test/unit/uiautomator2-specs.js
Outdated
| systemPort: 4724, | ||
| host: 'localhost', | ||
| devicePort: 6790, | ||
| disableWindowAnimation: false, |
There was a problem hiding this comment.
do we need these specific capabilities for unit tests? they are not used anyway
test/unit/uiautomator2-specs.js
Outdated
| uiautomator2.shouldUninstallServerPackages([ | ||
| { | ||
| 'installState': adb.APP_INSTALL_STATE.NOT_INSTALLED, | ||
| 'appPath': 'path/to/appium-uiautomator2-server.apk', |
There was a problem hiding this comment.
I assume these items should also be replaced by constants like it was done above
test/unit/uiautomator2-specs.js
Outdated
| uiautomator2.shouldInstall([ | ||
| { | ||
| 'installState': adb.APP_INSTALL_STATE.NOT_INSTALLED, | ||
| 'appPath': 'path/to/appium-uiautomator2-server.apk', |
lib/uiautomator2.js
Outdated
| @@ -97,6 +97,51 @@ class UiAutomator2Server { | |||
| return resultInfo; | |||
| } | |||
|
|
|||
| /** | |||
| * @typedef {Object} PackageInfo | |||
| * @property {string} installState | |||
There was a problem hiding this comment.
maybe for the installState we can set the type to typeof this.adb.APP_INSTALL_STATE and thus avoid uncessary conversions to string
lib/uiautomator2.js
Outdated
| // Enforce server packages reinstall if any of the packages is not installed, while the other is | ||
| const shouldUninstallServerPackages = (packagesInfo.some(({installState}) => installState === this.adb.APP_INSTALL_STATE.NOT_INSTALLED) | ||
| && !packagesInfo.every(({installState}) => installState === this.adb.APP_INSTALL_STATE.NOT_INSTALLED)); | ||
| // To ensure if the UIA2 driver should uninstall UI2 server packages from the device |
There was a problem hiding this comment.
This comment is now not really needed since we have the method itself properly documented
mykola-mokhnach
approved these changes
Jun 21, 2024
github-actions bot
pushed a commit
that referenced
this pull request
Jun 22, 2024
## [3.5.7](v3.5.6...v3.5.7) (2024-06-22) ### Bug Fixes * uninstall installed uia2 servers if they were greater than on-going session ([#796](#796)) ([62b9056](62b9056))
|
🎉 This PR is included in version 3.5.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
appium/appium#19799 (comment)
The current UIA2 driver does not uninstall UIA2 servers on the device if they are newer than newly installed servers by the UIA2. It could cause old UIA2 driver with newer UIA2 servers on the device, which is a mismatch condition. It could cause unexpected behavior.
Maybe...
this.adb.APP_INSTALL_STATE.NOT_INSTALLEDcondition should bethis.adb.APP_INSTALL_STATE.NEWER_VERSION_INSTALLEDto "uninstall" installed UIA2 servers?It looks like
sinonmock'sexpects,never, once etc might not be so reliable. Some could be not good. To improve condition check as test code, I have extracted condition check into two methods and added tests for them. At leastshouldUninstallServerPackagesandcanInstallServerPackagestests should be reliable.