-
Notifications
You must be signed in to change notification settings - Fork 914
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
Add async support to KIFUIViewTestActor #1275
base: master
Are you sure you want to change the base?
Conversation
@@ -162,25 +163,54 @@ - (BOOL)acknowledgeSystemAlert; | |||
{ | |||
return [self.actor acknowledgeSystemAlert]; | |||
} | |||
|
|||
- (void)acknowledgeSystemAlertWithCompletionHandler:(void (^)(BOOL, NSError*))completionHandler { | |||
@try { |
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 will likely lead to a memory leak in the test.
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 we should just let it fail if the error comes in like this
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.
Will the exception be caught by the XCTTestCase and continue running the next test, or will it crash the app?
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.
Is it possible to do that @justinseanmartin suggested here with a Also could we move these completionHandler's for async calls to a swift extension that is only accessible by swift? Objective-c shouldn't really ever need to use these methods nor should they AFAIK (could be wrong 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.
In regards to @dostrander's comment of adding a usingAsync:(BOOL)
as I'd originally suggested, is the part of this that is necessary to get async to work properly wrapping the actions in dispatch_async(dispatch_get_main_queue()
? Or is accepting a completionBlock
from the action caller also necessary? The latter seems a bit more complicated to do in a clean way, though I have some ideas (e.g. store the action as a block on the actor, then add a performAsyncActionWithCompletion:
method to perform it). There are probably better ways than that to handle it.
Would you be able to add an integration test that demonstrates how this is expected to be used? I think that might help for figuring out a shape of the API that doesn't also require duplicating all the actor's action methods. Thanks!
@@ -162,25 +163,54 @@ - (BOOL)acknowledgeSystemAlert; | |||
{ | |||
return [self.actor acknowledgeSystemAlert]; | |||
} | |||
|
|||
- (void)acknowledgeSystemAlertWithCompletionHandler:(void (^)(BOOL, NSError*))completionHandler { | |||
@try { |
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.
Will the exception be caught by the XCTTestCase and continue running the next test, or will it crash the app?
Thanks for the feedback! How it would look from the caller side using the
I can try do the change. I have some ideas about how I can do it, but i think it will be a bigger change. I will add a async test in swift to show the problems. About the completion block and the dispatch queue: I will test a little more. I notice that in some cases we don't need the About writing it in Swift: SPM doesn't support Swift and Objective C in the same package. I would be possible to create another package with all Objective C code, and then a package with the Swift code, but usually it brings more problems. |
Yea, I think what you'd listed is what I think it would probably look like. The other option is if many/all actions are expected to be done async, we could set it statically on the actor, like we do for some options such as the I wonder if the reason you need it sometimes and don't in others is whether it had to spin the runloop before finding the element that it was expecting to interact with? I suppose what you're doing with your changes is ensuring that there is always at least one spin of the runloop before starting to try and find the element on the screen. It probably wouldn't hurt to always do that, but only doing it when needed for async callers is fine too. The harder problem seems like figuring out how to incorporate a completion block into each of these. I haven't used SwiftUI with async/await, so I'm not quite sure on the requirements here. Having a small example (via a test) in the repo would help me contextualize it. |
As we talked in #1273, I tried to implement a possible solution.
Adding
CF_SWIFT_UNAVAILABLE_FROM_ASYNC("use async version");
to the methods, force the user to use the async version when in an async context.Because we dispatch to the main queue, exceptions are not captured by the test, and bubble up to the root of the project. To stop it, I added a
@try @catch
and return the errors in the completion block. It force the user to usetry await
for all calls, but I don't see other way around it.If you are ok, I will add the docs for the new functions later (basically copy and paste). Please let me know if I should change anything.