wix / Detox

Gray box end-to-end testing and automation framework for mobile apps
https://wix.github.io/Detox/
MIT License
11.01k stars 1.9k forks source link

feat(iOS): add system-dialogs interaction support. #4457

Closed asafkorem closed 1 week ago

asafkorem commented 1 month ago

Resolves: #4433, #4434, #4435, https://github.com/wix/Detox/issues/4275, https://github.com/wix/Detox/issues/3227, https://github.com/wix/Detox/issues/3017

This PR add support for system-dialogs interactions (system alerts and permission requests).

There are several changes in this PR:

Check the system test suite for example.

Check the rationale behind the new API design decision here.

noomorph commented 1 month ago

The elephant in the room (i.e. the longest answer to write) is:

This pull request current lacks elaboration on: 1) why by.system and system.element are the best possible solution in today’s context, 2) what were the other solutions considered, 3) why the latter are less suitable or subpar.

It’s unlikely you’ll be happy to write an essay on this, but this kind of engineering notes really helps colleagues and fellow contributors, so please 🙏

asafkorem commented 1 month ago

The elephant in the room (i.e. the longest answer to write) is:

This pull request current lacks elaboration on:

  1. why by.system and system.element are the best possible solution in today’s context,
  2. what were the other solutions considered,
  3. why the latter are less suitable or subpar.

It’s unlikely you’ll be happy to write an essay on this, but this kind of engineering notes really helps colleagues and fellow contributors, so please 🙏

Gladly, let's start by addressing the elephant in the room:

Why by.system and system.element?

These APIs specifically cater to system-level interactions, distinguishing them from other types of element interactions. This clear separation enables targeted, system-level access for elements outside the app, such as system alerts, where our basic app-level methods are insufficient. Additionally, by.system and system.element introduce an implicit natural priority for API calls. This prioritization ensures that system-level methods are called only when necessary, making the interaction pattern more declarative and streamlined for users.

API design decision

By aligning by.system with our by.web approach, we maintain a consistent and modular design philosophy. Each API is tailored for specific interaction domains, simplifying understanding, usage, and maintenance while avoiding unnecessary performance overhead by utilizing XCUITest only when absolutely necessary.

Considered alternatives (Integration with existing APIs)

The main alternative I considered was using a fallback mechanism - where interactions would automatically escalate to system-level (using XCUITest) if no elements were found. This approach introduced significant complexity and performance degradation, requiring a first check using the injected framework, then XCUITest.

In my opinion, any other "seamless" integration with our current APIs without explicitly specifying the system level would have blurred the operational clarity of our APIs.

Another approach considered was introducing another matcher withSystem() / onSystemContext(), which is somewhat similar to withAncestor(..) & withDescendant(..) which positions the matcher within some specific hierarchy. However, I decided it's better to align with by.web since withAncestor allows integration with all other matchers, and our current system matchers are not even a subset (i.e., by.type uses the accessibility type accessible through XCUITest, not the element class as we do within the app interactions context). Also, withAncestor only makes the matching more specific and does not change the entire context as onSystemContext() (or similar) would do.

Additionally, it's important to note that similar considerations apply to the future integration of UIAutomator (UIDevice) with this API on Android (i.e. considerations that are related to XCUITest).

To summarize, this system-focused approach ensures we do not introduce any performance degradation and making this solution more maintainable and specific for system-interaction needs and abilities. I hope this clarifies the rationale behind my design decisions.

noomorph commented 1 month ago

@asafkorem, I'm particularly concerned about the next things:

1) How well does it translate to Android? Will there be 1:1 system.element and by.system mapping, and will it be needed at all? For example, the way you're adding the XCUI test driver interface doesn't align with the Android counterpart, UIAutomator:

https://wix.github.io/Detox/docs/api/device/#devicegetuidevice-android-only

We should understand how we align `device.getUiDevice()` and `system.element` – either UIAutomator gets a rewrite to `*.system.*`, or XCUITest gets implemented for iOS in `device.getUiDevice()`, or something like that.

2) How sure are we that the XCUI test driver won't be needed elsewhere except for interacting with system dialogs? As we found out recently, cross-iframe WebView interactions can be done only via XCUI. Do you intend to mix web and system APIs in that case, e.g.:?

await web.element(by.system.type('button')).tap()

3) Are blind (x,y) taps and swipes on the device screen also an example of system elements and actions, in your opinion?

asafkorem commented 1 month ago

Unless there was a really good reason to do so, I find it pointless to inflate the CLI toolkit with typical parameterless commands like:

  1. build-framework-cache
  2. build-xcuitest-cache
  3. rebuild-framework-cache
  4. rebuild-xcuitest-cache

I see that it created a cascade of changes in documentation, and quite many copy&pastes between files.

First of all, I don't see significant value in exposing these implementation details to user on top level. Our users may be more than happy to live in blissful ignorance about what exactly the magical build-framework-cache command does in terms of troubleshooting.

Secondly, I recognize the importance of isolating the build/cleanup for these different projects, and this is why I'd suggest introducing a couple of useful CLI arguments:

detox build-framework-cache --xcuitest # build only XCUITest
detox build-framework-cache --detox # build only Detox.framework
detox build-framework-cache --clean # clean before rebuilding this or that

In that case:

  • build-xcuitest-cache, clean-xcuitest-cache, rebuild-xcuitest-cache can be omitted
  • rebuild-framework-cache can serve as a mere alias tobuild-framework-cache with an extra --clean option
  • maybe even clean-framework-cache can reuse build-framework-cache with (clean=true, xcuitest=false, detox=false), if this doesn't seem "too clever" to maintain.
  • we won't be adding any new CLI commands
  • just small additions to the docs will be required

I will be adding a few more notes but overall I must admit that this a very clean and nice PR. I decided not to wait with other notes, because this one implies some bit of rework, and the earlier you see it the better for us all it is.

xcuitest-cache commands were removed, and replaced with args for build-framework-cache and extended behaviour.

New options (args for detox-build-framework):

detox clean-framework-cache and detox rebuild-framework-cache are now aliases for detox build-framework-cache with specific flags.

asafkorem commented 1 month ago

@asafkorem, I'm particularly concerned about the next things:

  1. How well does it translate to Android? Will there be 1:1 system.element and by.system mapping, and will it be needed at all? For example, the way you're adding the XCUI test driver interface doesn't align with the Android counterpart, UIAutomator: https://wix.github.io/Detox/docs/api/device/#devicegetuidevice-android-only We should understand how we align device.getUiDevice() and system.element – either UIAutomator gets a rewrite to *.system.*, or XCUITest gets implemented for iOS in device.getUiDevice(), or something like that.

The current getUIDevice API is specific to Android and not aligned with our goal for a cohesive API across platforms. It essentially wraps Android's UIAutomator, which doesn't fit well (at all) with Detox's design. Instead of seeking a direct alignment with this, I plan to treat UIDevice as an underlying detail for the System APIs on Android. Both feature parity and clean API design are goals here.

  1. How sure are we that the XCUI test driver won't be needed elsewhere except for interacting with system dialogs? As we found out recently, cross-iframe WebView interactions can be done only via XCUI. Do you intend to mix web and system APIs in that case, e.g.:?
await web.element(by.system.type('button')).tap()

System APIs should definitely provide more than interactions with system dialogs.

My approach is to expand system APIs to include functionalities like push notifications, photo library interactions, system browsers (Safari, Chrome), and interactions with complex WebViews within apps. Regarding special webview cases, you raised a nice idea (to mix between system matcher and web-view apis), another solution will be to extend the current (and future) system API abilities to match and interact with elements within the app. IMO that's a discussion for itself. Anyway we will definitely leverage the new system abilities to interact with web-views in special scenarios as we are already familiar with one within the company.

  1. Are blind (x,y) taps and swipes on the device screen also an example of system elements and actions, in your opinion?

First, we need to evaluate if there are valid use cases that require blind interactions on the device screen.

Although these interactions are performed on system level, integrating them into the System API could complicate its design. The Device API, already provides a good abstraction for these types of gestures (special kind of system element with unique actions). Therefore, implementing those blind actions within the device object will help to maintain a clear distinction in functionalities although the overlapping aspects of device and system interactions.

d4vidi commented 3 weeks ago

Following up on the comment explaining the reasoning behind the API, I'd like to propose another approach that would offer some alleviation to the "does the API align with Android / future platform?" problem:

While the element.system approach offers a very powerful set of API's, it does, to some degree, leak the fact that some elements are system-oriented while other are app-based. This is indeed not necessarily coherent across all platforms, and should not - ideally, be of any interest to the Detox user.

In the price of more boilerplate work, we could go a different, more object-oriented, way and extend the API with mini-drivers that are tailored to specific usage cases. For example, a permissions dialog can be a Detox built-in mini-driver, that offers actions for acceping or denying the permission:

await device.permissionDialog.accept();
await device.permissionDialog.deny();
// And maybe also:
await expect(device.permissionDialog).toBeVisible();

Similarly, we could do this for other system-ish elements, such as system alerts, toasts (android), and even the photo gallery / camera. To some extent, this approach has already been introduced (implicitly) by specifying a dedicated API for interacting with date-pickers (Also true for Espresso, not just Detox).

While slightly cumbersome for us as project maintainers, this approach does force the mechanics of system interaction vs. app interaction onto mere implementation details, hidden from the user and cross-platform friendly. It therefore has the potential of being more sustainable for the user, all-the-while more straightforward (arguably).


UPDATE: I'd like to separate the discussion to 2 different questions: "What would be the right ultimate solution for the project?" and "What's the right way to get started and execute the long-term plan?". I'm still well opinionated that offering a system API as proposed could be a good start, especially if clearly proposed as an experimental API. It's still a good idea to start collecting mileage and learn as we seek to implement things on Android, on the go.

asafkorem commented 2 weeks ago

I'll mark this API as experimental, so it will be subject to changes at least until we collect the initial feedback. Thanks @d4vidi for this suggestion.

Den1sKa-aNd1 commented 2 weeks ago

Good day,

Any ETA on this?