feat(iOS): add system-dialogs interaction support. #4457
Conversation
asafkorem
force-pushed
the
feat/initial-xcuitest-integration
branch
from
193c43a
to
d1c9e1e
Compare
asafkorem
force-pushed
the
feat/initial-xcuitest-integration
branch
from
70bc7b5
to
7bfba22
Compare
|
The elephant in the room (i.e. the longest answer to write) is: This pull request current lacks elaboration on:
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 🙏 |
There was a problem hiding this comment.
Unless there was a really good reason to do so, I find it pointless to inflate the CLI toolkit with typical parameterless commands like:
- build-framework-cache
- build-xcuitest-cache
- rebuild-framework-cache
- 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 to
build-framework-cachewith an extra--cleanoption - 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.
Gladly, let's start by addressing the elephant in the room: Why
|
|
@asafkorem, I'm particularly concerned about the next things:
await web.element(by.system.type('button')).tap()
|
xcuitest-cache commands were removed, and replaced with args for New options (args for detox-build-framework):
|
The current
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.
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. |
asafkorem
force-pushed
the
feat/initial-xcuitest-integration
branch
from
7f7b2e7
to
37a0ee4
Compare
| const frameworkPath = path.join(os.homedir(), '/Library/Detox/ios/framework'); | ||
| const xcuitestPath = path.join(os.homedir(), '/Library/Detox/ios/xcuitest-runner'); | ||
|
|
||
| await handleCleaning( |
There was a problem hiding this comment.
Uh-oh... 😢
Actually, what I meant was:
- to call
handleBuildinginbuild-framework-cache - to call
handleCleaninginclean-framework-cache - to call both
handleCleaningandhandleBuildinginrebuild-framework-cache
I envisioned that those utility functions will reside in separate files, and be imported by each CLI tool independently, instead of dirty tricks with using yargs while you are using yargs:
const argv = hideBin(process.argv);
yargs(argv)
.command(require('./build-framework-cache'))
.parse(['build-framework-cache', '--clean="all"', '--build="all"']);In such case, the implementation should have been dead simple:
handleCleaning({ xcui: boolean; framework: boolean; }) => Promise<void>
handleBuilding({ xcui: boolean; framework: boolean; }) => Promise<void>
Sorry for nitpicking, but wouldn't detox (re|build|clean)-framework-cache --this --that --those look and feel more natural and simple?
There was a problem hiding this comment.
I mean that this API doesn't make sense to me:
--build: Specifies binaries to build (all / detox / xcuitest / none). Defaults to all.
--clean: Specifies binaries to clean (all / detox / xcuitest / none). Defaults to none.
What practical sense does it bear to call detox ... --build xcuitest --clean detox ?
To my understanding, --clean is a boolean flag which is attributed to every target in the question, be it all, detox, or xcuitest. There's no sense to attach "clean logic" to something that is not being built or rebuilt.
I think that
detox build-framework-cache --clean --xcuitest --detox
or
detox ... build-framework-cache --clean --target=xcuitest,detox would make more sense.
For implementing the latter, you can check coerce function in yargs, where you can post-process the value.
There was a problem hiding this comment.
For detox build-framework-cache --clean --xcuitest --detox, the default setting should enable both xcuitest and detox enabled. However, if only one of these options is specified, the other should be disabled. This setup may not be very intuitive, but I'm okay with it. I'd prefer not to spend too much time on discussing this internal utility to avoid bike-shedding. I'm totally fine with that, will change
There was a problem hiding this comment.
Actually, something bothers me. What's the point of adding the --clean flag here if we have the rebuild-... command? It's like using --build from clean-...
I'll just add the --xcuitest and --detox flags for all commands, and the clean / rebuild command will call the clean method.
| @@ -1,76 +0,0 @@ | |||
| #!/bin/bash -e | |||
There was a problem hiding this comment.
@asafkorem I wonder, won't removing legacy scripts break someone? Who's our vulnerable audience?
There was a problem hiding this comment.
Who's our vulnerable audience?
Allegedly, Xcode < 12 if such thing still exist (current Xcode version is 15), though they can update their xcodebuild (CLI) version and they'll be fine.
Xcode 11 is quite outdated (supports only iOS <= 13). Xcode 12 was released in 2020, and I have no confidence that Detox will compile on such an old version (it also comes with the respective old simctl and xcodebuild command).
IMO there is no good reason to continue maintaining or supporting this script
| ]; | ||
|
|
||
| try { | ||
| return await exec(`TEST_RUNNER_PARAMS="${base64InvocationParams}" xcodebuild ${flags.join(' ')}`); |
There was a problem hiding this comment.
Can we pass TEST_RUNNER_PARAMS to { env } parameter to exec?
There was a problem hiding this comment.
Uh, I saw your comment about this. This is not critical, but I'll check later why it fails. I just really don't want to pollute detox.trace.log, so I'm rather persistent and returning to this every time. 😊
detox/src/ios/system.js
Outdated
| } | ||
|
|
||
| expect(expectation, traceDescription) { | ||
| assert(traceDescription, `must provide trace description for expectation: \n ${JSON.stringify(expectation)}`); |
There was a problem hiding this comment.
I feel uneasy about eager object stringifying each time we try to send something. To my embarrassment, I would even be fine with stupid if (!traceDescription) { assert(...) }, just not to spend CPU time for things that won't be needed in 99.99% cases.
I see two good options:
- check if stack traces are usable even without this stringification – this really can be so
- if not, use homebrewed assertion utils from
./src/utils(if needed, add some), or add a helper function somewhere below maybe... 🤷♂️
Sorry for nitpicking. 🙏
There was a problem hiding this comment.
I wonder if some workaround like
const lazyMessage = {
toString: function() {
return `... ${JSON.stringify(...)}`;
}
};will do the trick when passing it to the assert function. however that's a bit dirty.
There was a problem hiding this comment.
you're right about 1, the JSON is redundant here.. there's enough information without it
detox/src/utils/environment.js
Outdated
| @@ -171,24 +172,36 @@ function throwMissingGmsaasError() { | |||
| throw new DetoxRuntimeError(`Failed to locate Genymotion's gmsaas executable. Please add it to your $PATH variable!\nPATH is currently set to: ${process.env.PATH}`); | |||
| } | |||
|
|
|||
| function getDetoxVersion() { | |||
| const getDetoxVersion = _.memoize(() => { | |||
There was a problem hiding this comment.
Rather a nitpicking, but you don't need to use memoize here and all the way down the file. You'll be absolutely fine with _.once since your functions don't require any arguments. It is a more lightweight implementation, and its mental model is a bit easier/lighter.
There was a problem hiding this comment.
oh, great. replaced
Since title is being hidden by the "Active" / "Busy" mark.
seems like a bug in istanbul
asafkorem
force-pushed
the
feat/initial-xcuitest-integration
branch
from
a5e3348
to
7cea0f4
Compare
Resolves: #4433, #4434, #4435, #4275, #3227, #3017
This PR add support for system-dialogs interactions (system alerts and permission requests).
There are several changes in this PR:
xcuitest-cache-build/rebuild/clean), and refactored existing scripts.by.system.label,by.system.type.tap()).toExist(),atIndex(..),notmodifier).Check the
systemtest suite for example.Check the rationale behind the new API design decision here.