-
-
Notifications
You must be signed in to change notification settings - Fork 612
chore(e2e): maintenance for automated tests after iOS 26.2 #3430
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?
chore(e2e): maintenance for automated tests after iOS 26.2 #3430
Conversation
a48c2f0 to
e82c68e
Compare
t0maboro
left a comment
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.
left some questions
d48d998 to
2401b65
Compare
kkafar
left a comment
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.
Hey! Thanks for the PR. I've left few questions & remarks. Please answer them (or let us know that the PR needs an assignee).
| describe('Bottom tabs and native stack', () => { | ||
| beforeEach(async () => { | ||
| await device.reloadReactNative(); | ||
| await device.launchApp({ newInstance: 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.
I have a question here. Why do we relaunch application instead of reloading RN? This was done mostly to save CI time IIRC, so it is a subject to be changed, but I need reasoning here.
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, I discovered that the Example ("Paper") app behaves differently: the app may start ignoring touch events (the problem is most clearly visible on the SettingsPicker from apps\src\screens\Events.tsx:77, unluckly I lost the video with the proof). I don't know if the issue is in the react-native-screens or just in the test app. The logcat looked fine. I didn't have time to reproduce the exact conditions of the problem. But it [always] appeared only in these test suites (in different test cases) and only with automated executions (I failed with reproducing it manually, even accidentally).
Existing tests passed, when I added device.launchApp({ newInstance: true }); which increases their isolation.
| beforeEach(async () => { | ||
| await device.reloadReactNative(); | ||
| // await device.launchApp({ newInstance: true }); | ||
| await device.launchApp({ newInstance: 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.
Same here, and in every other place, where it has been done.
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 wonder about naming here. What are component-objects? Shouldn't be this kind of util directory with locators.js / actions.js?
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, we can discuss that:
One of the top industry standards for e2e and ui automated tests is Page Object Model (a.k.a. Page Object Pattern) which separates behavior from a technical selectors and operations. This pattern is about having separate
test case files (the higher abstraction level, ideally even without touching automation library, like Selenium, Playwright, Detox etc),
"Page Object" files which define functions used be test cases (a middle abstraction layer that "maps" the elements and actions needed to execute the test with the automation library actions, so basically hiding all "technicall" elements(by.id(... and .tap() from test scenario)
and of course autamation library is the lowest abstraction layer in this pattern.
I tried also to re-invent wheel here as these Page Object files group actions and selectors depending on the page on which they are available (e.g. login/email and password inputs would be in the page-object/login.ts file) which is not usefull for us. I came up with an idea to shift focus on reusable componenst rather than pages...
Right now, there's only this header back button.
Should I move it and change the name (like e2e/component-objects/back-button.ts -> e2e/actions.ts)?
| } | ||
| async function getIOSBackButton() { | ||
| const iosVersion = semverCoerce(getIOSVersion().replace('iOS', ''))!; | ||
| if (semverSatisfies(iosVersion, '>=26.0')) { |
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.
Does this function work only on iOS 26+?
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 function exists for iOS26+ but returns just the element(by.id('BackButton')) for other versions (line 34)
| await element(by.id('home-button-open-second')).tap(); | ||
|
|
||
| await element(by.id('BackButton')).tap(); | ||
| await tapBarBackButton(); |
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 is an iOS only test. We have nice & clean way to detect a back button & tap it & we replace it with the complicated logic in component-objects/back-button.ts? Why do we do this? My guess would be that this version does not work, but I need confirmation & explanation.
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, selector element(by.id('BackButton')) resolves to two elements since iOS 26+ so taping on it with Detox causes an error.
Adding an assignee could be helpful: I can answer in threads but this PR belongs directly to react-native-screens and I cannot change its code anymore. That means I need someone else to resolve conflicts (it seems we have some already) and merge the PR at the end. I can apply suggestions only by PRs from a fork (like I do in #3451). The alternative is to close this PR and I could open a new one - entirely as a fork PR |
d3d7f11 to
6fb0bec
Compare
|
Cleaned up the PR a bit
I keep the simulated scrolling instead of searching on iOS for now - this PR focuses only on making the CI complete successfully. This will be rewritten in a followup after #3532 and I will make the search work there. |
|
Okay, so the CI passes, but there are lots of skipped tests (and I'm adding another one here, Test2809 which should be updated after #3303 anyways). Also there are .js files which are imported in TS and need .d.ts declarations - should we just rewrite them to TS? And there is still the question about file structure (see the comment above). |
646238f to
97a0f5a
Compare
Co-authored-by: Krzysztof Ligarski <63918941+kligarski@users.noreply.github.com>
This reverts commit fee8faf.
97a0f5a to
bffc313
Compare
Description
Closes https://github.com/software-mansion/react-native-screens-labs/issues/540
Broad changes after iOS 26.2 and some by-the-way fixes or improvements.
Not fixed tests:
iOS tests for example Test2809 should be aligned later with an output of PR #3303.
iOS tests for examples Test645 and Test649 are affected by issue #566
Changes
Detox e2e tests. Increased test isolation. Bar back button technical selector depends now on ios version.
Some android emulators my get a pop-up about using stylus which interrupts the test flow – it is disabled in the global config now.
Checklist