Skip to content

Conversation

@KrzysztofWojnar
Copy link
Contributor

@KrzysztofWojnar KrzysztofWojnar commented Nov 27, 2025

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

@KrzysztofWojnar KrzysztofWojnar requested review from kkafar, kligarski, kmichalikk and t0maboro and removed request for kkafar and t0maboro November 27, 2025 18:44
@KrzysztofWojnar KrzysztofWojnar force-pushed the @KrzysztofWojnar/e2e-test-maintenance-ios-26.2 branch from a48c2f0 to e82c68e Compare November 28, 2025 10:09
Copy link
Contributor

@t0maboro t0maboro left a comment

Choose a reason for hiding this comment

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

left some questions

@KrzysztofWojnar KrzysztofWojnar changed the title WIP:chore(e2e): automated test maintenance for iOS 26.2 chore(e2e): automated test maintenance for iOS 26.2 Nov 28, 2025
@KrzysztofWojnar KrzysztofWojnar changed the title chore(e2e): automated test maintenance for iOS 26.2 chore(e2e): maintenance for automated tests after iOS 26.2 Nov 28, 2025
@KrzysztofWojnar KrzysztofWojnar force-pushed the @KrzysztofWojnar/e2e-test-maintenance-ios-26.2 branch from d48d998 to 2401b65 Compare November 28, 2025 11:52
Comment on lines +21 to +23
(await backButtonElement.getAttributes()) as unknown as {
elements: { className: string }[];
};
Copy link
Contributor

Choose a reason for hiding this comment

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

await backButtonElement.getAttributes() returns Detox.IosElementAttributes | Detox.AndroidElementAttributes | { elements: Detox.IosElementAttributes[]; } | { elements: Detox.AndroidElementAttributes[]; }, maybe we can handle this type somehow instead of forcing a type.

Copy link
Contributor Author

@KrzysztofWojnar KrzysztofWojnar Dec 1, 2025

Choose a reason for hiding this comment

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

That's the tough one.
It is an iOS-version-specific, behavior-based implementation for a technical selector, BUT QUITE READABLE (comparing to my other options). The biggest problem with the 'className' field is that it is not documented (I'm using the Hyrum's law).
It may break in future versions, but I can't predict it and finding this header back button in another way (filtering by other properties) is equally uncertain (or may break even easier). I don't have more robust solutions, you can call it a skill issue.
This probably won't make you feel any better, but I don't like it either.

Copy link
Contributor Author

@KrzysztofWojnar KrzysztofWojnar Dec 1, 2025

Choose a reason for hiding this comment

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

Oh, I can use magic numbers alternatively (backButtonElement.atIndex(0)).
I bet most people would do this in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see what you mean. I think the current version is all right then.

Comment on lines +18 to +19
const iosVersion = semverCoerce(getIOSVersion().replace('iOS', ''))!;
if (semverSatisfies(iosVersion, '>=26.0')) {
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 wondering if we need semver for this? On the other hand, it's just the example app so I guess that it's okay.

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 wanted to save some time and several lines of code by adding the dev dependency, but I agree - it is an overkill and we can parse MAJOR.MINOR (or just bare MAJOR) oursevles. I don't have a strong opinion about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @kkafar

Copy link
Member

Choose a reason for hiding this comment

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

No, we don't need external dependency for this. Simple helper function is enough for that. Given what is going on with NPM & supply chain attacks recently -> lets avoid any unnecessary dependencies.

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 am not able to push to this repository anymore, so I applied this suggestion in the fork:
#3451
(If I meesed up something with github like forking and pull requesting, please let me know too)

`} catch {` and `} catch (_) {` it's pretty the same, "(_)" shows we intentionally skip the error handling, but skipping it is just more concise

Co-authored-by: Krzysztof Ligarski <63918941+kligarski@users.noreply.github.com>
Copy link
Member

@kkafar kkafar left a 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 });
Copy link
Member

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.

Copy link
Contributor Author

@KrzysztofWojnar KrzysztofWojnar Dec 9, 2025

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 });
Copy link
Member

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.

Comment on lines +18 to +19
const iosVersion = semverCoerce(getIOSVersion().replace('iOS', ''))!;
if (semverSatisfies(iosVersion, '>=26.0')) {
Copy link
Member

Choose a reason for hiding this comment

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

No, we don't need external dependency for this. Simple helper function is enough for that. Given what is going on with NPM & supply chain attacks recently -> lets avoid any unnecessary dependencies.

Copy link
Member

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?

Copy link
Contributor Author

@KrzysztofWojnar KrzysztofWojnar Dec 9, 2025

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 technicall 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 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')) {
Copy link
Member

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+?

Copy link
Contributor Author

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();
Copy link
Member

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.

Copy link
Contributor Author

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.

@KrzysztofWojnar
Copy link
Contributor Author

KrzysztofWojnar commented Dec 9, 2025

@kkafa

Hey! Thanks for the PR. I've left few questions & remarks. Please answer them (or let us know that the PR needs an assignee).

Adding an assignee could be helpful: I can answer in the 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

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.

5 participants