-
Notifications
You must be signed in to change notification settings - Fork 4
iOS media handling changes #42
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
base: main
Are you sure you want to change the base?
Conversation
run/types/DeviceWrapper.ts
Outdated
// Checking Sent status on both platforms | ||
await this.waitForTextElementToBePresent({ | ||
...new OutgoingMessageStatusSent(this).build(), | ||
maxWait: 50000, |
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.
50s?
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 should be 20s now
run/test/specs/utils/open_app.ts
Outdated
// Simulators are cleared on start of a test run to minimize device bloat | ||
// This has to be done before starting them because you can't erase booted simulators | ||
// If a simulator is already running this fails silently | ||
await runScriptAndLog(`xcrun simctl erase ${udid}`); |
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.
for many tests/simulators do we need to erase the data? Just wondering if there is a better way to do this, more reliably.
As in, we always push the file/media from the first device for each tests that need it, right?
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.
As discussed, I think this may need to be removed. The enormous time bloat is far outweighing the problem of having a flaky locator for media.
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 working on a different approach that could allow us to not rely on any set dates for accessibility IDs (which was the whole point of this refactor)
If that works out (🤞🏻) we might get away with having a simple cronjob on the CI that periodically wipes all iOS simulators, just to minimize device bloat.
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 removed this, we no longer wipe iOS simulators on start
run/test/specs/utils/open_app.ts
Outdated
// Simulators are cleared on start of a test run to minimize device bloat | ||
// This has to be done before starting them because you can't erase booted simulators | ||
// If a simulator is already running this fails silently | ||
await runScriptAndLog(`xcrun simctl erase ${udid}`); |
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.
As discussed, I think this may need to be removed. The enormous time bloat is far outweighing the problem of having a flaky locator for media.
} | ||
await alice1.onIOS().clickOnByAccessibilityID('Photo, 25 March, 11:09 am', 1000); | ||
await matchAndTapImage( | ||
alice1.onIOS(), |
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.
what is this onIOS for?
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.
also, should we move this function to the DeviceWrapper class directly?
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.
The partial image matching only needs to be done on iOS. Android can be wiped at runtime so we're still tapping the one and only image that's just been pushed.
I added the matchAndTapImage
to the DeviceWrapper
@@ -20,6 +20,7 @@ | |||
"devDependencies": { | |||
"@appium/execute-driver-plugin": "^3.0.1", | |||
"@appium/images-plugin": "^3.0.17", | |||
"@appium/opencv": "^3.0.9", |
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.
we cannot do the same with the image matching that playwright offers?
I guess not because
- this will not just look for a buffer that looks like another one,
- but will actually try to find in a buffer, a part that looks like another one. Is that it?
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.
Yes this is partial image matching
const center = { | ||
x: rect.x + Math.floor(rect.width / 2), | ||
y: rect.y + Math.floor(rect.height / 2), | ||
confidence: score, |
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.
the confidence field can be removed, right?
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.
Good catch, removed
Downloads
directory on disk