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

refactor: Get rid of the legacy quiescence control stuff #404

Merged
merged 29 commits into from
Oct 22, 2020

Conversation

mykola-mokhnach
Copy link

BREAKING CHANGE: the minimum supported Xcode SDK version is bumped to 10.2
BREAKING CHANGE: quiescence check is enabled by default (only breaking for WDA, because Appium does enable it by default)

This is a big change and it needs to be carefully tested on supported SDKs before being merged to master. Long story short, now the actual quiescence timeout is controlled by waitForIdleTimeout setting (same as in uia2, also measured is ms). Setting it to zero disables quiescence checks. The default value is 10 seconds (same as for Android).

@Dan-Maor @KazuCocoa I hope you could assist with testing

BREAKING CHANGE: the minimum supported Xcode SDK version is bumped to 10.2
@KazuCocoa
Copy link
Member

okay, will take a look. (It seems this should be after 1.19.0)

@mykola-mokhnach mykola-mokhnach changed the title refactor: Get rid of the legacy quiescence control stuff [WIP] refactor: Get rid of the legacy quiescence control stuff Oct 16, 2020
@Dan-Maor
Copy link
Collaborator

This may help to fix the testClickAlertButton test: in fb_resolveWithError, use:
- (void)resolve:(NSError**)arg1;
instead of:
- (void)resolve;
That way the resulting resolve error of the nonexisting alert element will not trigger an assertion failure, it will only be dumped into the error variable.
I have verified that testClickAlertButton passes locally with Xcode 10.2 when using this selector instead, however I did not run any further tests yet.

@mykola-mokhnach
Copy link
Author

Thanks a lot @Dan-Maor , I've applied the patch

@mykola-mokhnach mykola-mokhnach changed the title [WIP] refactor: Get rid of the legacy quiescence control stuff refactor: Get rid of the legacy quiescence control stuff Oct 17, 2020
@mykola-mokhnach
Copy link
Author

@Dan-Maor I've tuned the logic a bit and swizzled lower level methods named _notifyWhenMainRunLoopIsIdle: and _notifyWhenAnimationsAreIdle:. This approach should be more unified, since the methods are also called at application startup for quiescence check. I'm not quite sure about the incoming arguments of their callback blocks. While debugging it looks like they both contain nil

@Dan-Maor
Copy link
Collaborator

I think swizzling this deep might be a bit risky.

One thing I noticed was that the second argument of the callback is not an NSObject of any kind, it rather appears to be a pointer to something which I couldn't identify as of yet (need to look further into it, currently occupied with work tasks). As it's currently defined as id, WDA crashes if it's not nil. This is a scenario I was able to get to after some testing with simulating a slow app launch by lowering the waitForIdleTimeout. After modifying the second callback argument to (void*) WDA did not crash, however it did hang in createSession of the Clock application for a long time.

I'm afraid that the more we test we'll find more scenarios like the one outlined above. Therefore I really think we should stick to the internal XCTApplicationStateTimeout() (which holds 60.0 by default) and XCTSetApplicationStateTimeout(double timeout) functions and let the XCTest framework handle it for us.

@@ -70,4 +75,9 @@
// Gone with iOS 10.3
- (void)waitForQuiescence;

// Since Xcode 10.2
- (void)_notifyWhenAnimationsAreIdle:(CDUnknownBlockType)arg1;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We apparently know what the signature of those blocks is: void (^)(id, NSError *). Casting the IMP pointer would still be necessary in XCUIApplicationProcess+FBQuiescence.m, but we would have the same things defined in both places.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed the signature

Comment on lines 35 to 37
if (!isSignalSet) {
onIdle(sender, error);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still a race condition because isSignalSet can be changed on a different thread.
Could we just use dispatch_once in both places where onIdle(...) is called and get rid of isSignalSet?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added locks

Comment on lines 66 to 68
if (!isSignalSet) {
onIdle(sender, error);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here as above in swizzledNotifyWhenMainRunLoopIsIdle

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added locks

isSignalSet = YES;
[FBLogger logFmt:@"The application %@ is still waiting for being in idle state after %.3f seconds timeout. Making it to believe it is idling", [self bundleID], FBConfiguration.waitForIdleTimeout];
[FBLogger log:@"The timeout value could be customized via 'waitForIdleTimeout' setting"];
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the original callback also being executed on a background thread?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it should be always executed on a background thread

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.

5 participants