Skip to content
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

Closed

Conversation

ggilles
Copy link

@ggilles ggilles commented Oct 16, 2023

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 of react-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.

ggilles added a commit to ggilles/react-native-gesture-handler that referenced this pull request Oct 16, 2023
ggilles added a commit to ggilles/react-native-gesture-handler that referenced this pull request Oct 16, 2023
@noomorph
Copy link

noomorph commented Oct 18, 2023

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:

  1. doesn't guarantee that flakiness has not increased — that can be measured only through hundreds of test runs
  2. doesn't guarantee that other projects using Detox won't get less reliable.

Can we look in the opposite direction, and swizzle cancelPreviousPerformRequestsWithTarget instead?

@ggilles
Copy link
Author

ggilles commented Oct 19, 2023

@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 npm build:ios from the detox/test folder, and lerna bootstrap doesn't compile DetoxSync either: I checked this by introducing an obvious compile error in DetoxSync and couldn't make compilation fail from the Detox folder... 🤦

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 ?

@noomorph
Copy link

noomorph commented Oct 19, 2023

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.
I clearly see that his observation is trustworthy, and some action about swizzling is indeed required here.

@asafkorem
Copy link
Contributor

asafkorem commented Oct 19, 2023

@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 npm build:ios from the detox/test folder, and lerna bootstrap doesn't compile DetoxSync either: I checked this by introducing an obvious compile error in DetoxSync and couldn't make compilation fail from the Detox folder... 🤦

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 ?

Hey @ggilles, if you're testing Detox with our test app, you can run detox rebuild-framework-cache. This will rebuild the framework (including DetoxSync - of course, you need to make sure the git submodule is pointing on the relevant DetoxSync commit).

@asafkorem
Copy link
Contributor

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?

@ggilles
Copy link
Author

ggilles commented Oct 26, 2023

@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.

@asafkorem
Copy link
Contributor

@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.

@asafkorem
Copy link
Contributor

Here's the workaround: #69.
I will release a Detox version with this patch in the next couple of hours.

@asafkorem asafkorem closed this Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Taps on a View with RNGH do not work as expected when the app is started with Detox
3 participants