-
Notifications
You must be signed in to change notification settings - Fork 17
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
Remove performSelector spy #68
Remove performSelector spy #68
Conversation
Hmm, this is truly interesting, but I have concerns. Do you know what idling resources are and what they're for? I mean, the fact that this removal does not break the Detox E2E suite:
Can we look in the opposite direction, and swizzle |
@noomorph I think I understand why those resources were tracked initially, yes: The role of DetoxSync is to be able to tell when the device is busy or not, so that Detox can wait for the device to be idle before dispatching the next expectation/action. And it's true that this is quite a bold move. I suggested that change because effectively Native timers are already no longer properly tracked for iOS16+. This suggests that tracking those resources might not be so important in practice. However, I just realized my runs of the Detox E2E suites were wrong: the DetoxSync code is not compiled at all when doing So, it seems I should go back to testing this proper. Can you tell how to make Detox use a version of DetoxSync built from my branch ? |
Well, DetoxSync is a git submodule inside Detox, so you'd just have to set it to a correct revision, AFAIU. @asafkorem could you assess the situation around this PR, please, and recommend something to @ggilles. |
Hey @ggilles, if you're testing Detox with our test app, you can run |
Hey @ggilles, I'll begin looking into this issue today. I'm uncertain whether I'll adopt a similar method (removing part of our synchronization mechanism), but I'll likely attempt to sync with the calls we've missed It would be greatly beneficial if you could replicate this issue within a specific branch of our test-app. Including a failing test due to this issue will make it much easier. I'm curious whether it’s possible to reproduce this without integrating the gesture-handler library—could it be straightforward enough? |
@asafkorem I think it should be pretty straightforward to create a test for this, I am just short on time right now. It should be something along the lines of: - (void)triggerError
{
[self makeErrorVisible];
}
// This would be wired to a UI element so that Detox can fire it
- (void) triggerErrorThenCancel
{
// ask to trigger the error UI
[self performSelector:@selector(triggerError) withObject:nil afterDelay:1];
// and cancel that immediately
[NSObject cancelPreviousPerformRequestsWithTarget:self selector:@selector(triggerError) object:nil];
} And then on the Detox side you would trigger triggerErrorThenCancel and then expect that the error isn't visible. |
@ggilles, I was able to reproduce that. I will push a workaround specifically made for the RNGH case until we can address this with a more generic solution. Unfortunately, the future fix is not trivial. |
Here's the workaround: #69. |
This is the result of trying to fix wix/Detox#4194
I tracked down the issue to the fact that DetoxSync swizzles both NSObject:performSelector:withObject:afterDelay:inModes: and NSObject:performSelector:onThread:withObject:waitUntilDone:modes:, but it does not swizzle the associated cancel methods
NSObject:cancelPreviousPerformRequestsWithTarget
.This means that any app/library using
cancelPreviousPerformRequestsWithTarget
will perform differently when run using detox. This is the case for the single-tap handling ofreact-native-gesture-handler
and the result is that only the first single tap is correctly detected when run using detox.My proposal here is to simply remove the performSelector spy. It doesn't seem to affect the results of the Detox e2e test-suite.