-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat(iOS): add system-dialogs interaction support. #4457
Conversation
193c43a
to
d1c9e1e
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-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.
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 think I've finished the review for now. Thanks a lot for keeping it compact. Great job overall. See my comments. 🙏
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh-oh... 😢
Actually, what I meant was:
- to call
handleBuilding
inbuild-framework-cache
- to call
handleCleaning
inclean-framework-cache
- to call both
handleCleaning
andhandleBuilding
inrebuild-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asafkorem I wonder, won't removing legacy
scripts break someone? Who's our vulnerable audience?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we pass TEST_RUNNER_PARAMS
to { env }
parameter to exec?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, great. replaced
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 biggest concern is that CLI redirection logic between those scripts looks too heavy-weight and clumsy. Reusing and calling helper functions would have been much cleaner and simpler to comprehend. I seriously recommend reconsider that implementation. 🙏
seems like a bug in istanbul
a5e3348
to
7cea0f4
Compare
docs/api/system.md
Outdated
|
||
System APIs are currently in an experimental phase. | ||
This means that the API is not yet stable and may change in the near future. | ||
**Do not rely on the current API as it may change in future minor releases.** |
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 line Do not rely on this API
sounds almost like shooting in your own leg. Needs rephrasing. Maybe just mention that "This API may change over minor releases", because "Do not rely" sounds like "Please don't use 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.
hehe, valid point. ok, rephrasing
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.
Looks really good except one small thing in the system.md
article. ☝️
Hello people, thanks for your amazing work in advance. Unfortunately i dont get this system api work... Maybe i am stupid, but: I updated detox to v20.23.0 But, if i run a test: console.log(system); -> delivers undefined i tried to fix the issue via several ways: deleting node_modules, re add them The test runs, but "system" is still undefined, no matter what i am trying... what did i wrong? I appreciate your help, |
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(..)
,not
modifier).Check the
system
test suite for example.Check the rationale behind the new API design decision here.