Bump the recent fixes from FB WDA repository #134
Conversation
Summary: We have duplicated scrolling tests for regular tableViews and simple scrollviews. It was usefull to have them, but now testing scrolling on scrollviews covers both cases anyway and we will waste less time testing. Reviewed By: antiarchit Differential Revision: D6543548 fbshipit-source-id: 35e717c43208b8f138a706798d9aa8a9e34b302a
Summary: Some alert integration tests are flaky on our CI. I suspect this happens due to failing `tap` command, which might be failing to tap on animated view. This know XCTest issue. Fortunately we have `fb_tapWithError` that handels are common pitfalls. Reviewed By: antiarchit Differential Revision: D6543616 fbshipit-source-id: f12ea7da67058253deff53fa980ebf2e55a93899
Summary: Rather then depending on XCUITest information on orientation, we should test what is application perception Reviewed By: antiarchit Differential Revision: D6543511 fbshipit-source-id: 192e50d0202f96bd6119921c9353a72789b33ad4
Summary: This test is super device/simulator specific, that was used to interate on visibility improvements. Right now it does not bring much value anyway. Reviewed By: antiarchit Differential Revision: D6543627 fbshipit-source-id: ffbda3b0b5ecde6a632e88030e84fae5af614347
Summary: It is time to move on. Reviewed By: antiarchit Differential Revision: D6692223 fbshipit-source-id: 81913167551191b2f9e177ff1ead943f6fc25752
Summary: `XCUICoordinate` seems to have a bug that will return half of offset in case coordinate referance point goes off screen. I suspect this has something to do with retina displays. Instead of using Apple's implementation of `screenPoint` I implemented our own. Reviewed By: antiarchit Differential Revision: D6702218 fbshipit-source-id: b01ef061732281cdf8c6707de38a11b6e31b9c28
Summary: With all visibility changes tested pageindicator is no longer considered invisible. For the time being this is ok with us. This diff uses different invisible elements to run same test of search for invisible elements Reviewed By: antiarchit Differential Revision: D6702252 fbshipit-source-id: a2bd5198d14f05cb1b33afe76a6b5bda7e34a847
Summary: This test was not intended to run on iPad, so we should turn it off so that we can make Travis green again :). Reviewed By: antiarchit Differential Revision: D6726707 fbshipit-source-id: db92f23b2dc755e7c1464069c04b1838717b8ff4
Summary: With new xcode it looks like, calling `query`, `resolve` on XCUIApplication will in some sense break it. Not sure exactly what goes wrong, but final result is that it's missing some children and description of that object is empty. Instead we can use new API for fetching snapshots for debugging. Reviewed By: lawrencelomax Differential Revision: D6723102 fbshipit-source-id: a33d9caf03eef4450f3879996493493c2cb257b4
Summary: Using views frame for calculating scroling vector might be buggy when application is doing tricky stuff with scolling views eg. nesting scrollview within scrollview. In that case frame of the second scrollview will be huge and make scrolling imposible as we will try to use points of the screen. Instead we should visible frame. Reviewed By: antiarchit Differential Revision: D6819626 fbshipit-source-id: 27062f593485b4ec55ae8bd1a9c9a32a89630b92
Summary: [XCUIDevice sharedDevice].orientation may get values like FaceUp, which cause wrong calculation of the app frame size, in landscape. With devices in Lab, standing portrait, this problem is less common. Working on development with devices on the table it happens more often causing a great deal of confusion. From my tests, using app interfaceOrientation gives always the reality of the App. Closes facebookarchive#875 Reviewed By: marekcirkos Differential Revision: D7067153 Pulled By: antiarchit fbshipit-source-id: c6aa8f8823d0f43e5072a190c34df931d0d6b8b8
Summary: It might be there is more than one `IntegrationApp` icon on the dashboard while integration tests are running on Travis. Addresses https://travis-ci.org/facebook/WebDriverAgent/jobs/349725814 Closes facebookarchive#883 Differential Revision: D7289614 Pulled By: marekcirkos fbshipit-source-id: e5060888f2a752c039fcf815984e24579a3cef06
Reviewed By: antiarchit Differential Revision: D7615943 fbshipit-source-id: 9cb82dce23c02970d0c29613c7de4ebca0838361
Summary: Include RoutingHTTPServer in copy to fix ISSUE #902 Closes facebookarchive#915 Differential Revision: D7947944 Pulled By: marekcirkos fbshipit-source-id: ca60238fc3f715f753f084493aec5afa4524b21b
Summary: If SDK >= 11, the tap coordinate based on application is not correct when the application orientation is landscape and tapX > application portrait width or tapY > application portrait height. Pass the window element to the method [FBElementCommands gestureCoordinateWithCoordinate:element:] will resolve the problem. More details about the bug, please see the following issues: #705, #798, #856. Notice: On iOS 10, if the application is not launched by wda, no elements will be found. See issue #732. Closes facebookarchive#878 Differential Revision: D7947931 Pulled By: marekcirkos fbshipit-source-id: 4f8ef1484761b79c69f67df40293eea70bc08984
Differential Revision: D8220228 fbshipit-source-id: cf08317ab2e50282373576fedb7c2b26b47969ad
Summary: In case testmanagerd has problems in calculating frames it may return 'inf' instead which does not behave well with JSON encoding. Therfore checing for infs when formating JSON reponse Reviewed By: antiarchit Differential Revision: D7067158 fbshipit-source-id: 180400186af49f1353d93e9eb61d7306b8a1a0f6
Summary: A couple of commands which make possible to force touch given element or point by coordinates. In fact – copy of the tap extension. Baked with integration tests. Updated with mykola-mokhnach improvements in #79 Closes facebookarchive#917 Differential Revision: D8220249 Pulled By: marekcirkos fbshipit-source-id: 2a14ab5759894577a1f5e40f20b4a6d79e519419
Summary: Every time a user finds an element using the `element` or `elements` route, a new `XCUIElement` object is added to the session cache. The elements are never removed from the cache. A very naïve script which goes to the home screen and queries for the 'Phone' element 1000 times in a row, will cause memory consumption (as measured by Xcode) of the WebDriverAgent to grow to approximately 126 MB. Net, it appears that every `XCUIElement` object consumes about 100KB of memory. For sessions which persist for a long period of time (say, 30 minutes or more), this can eventually lead to an out of memory situation in which the WebDriverAgent crashes. This PR tries to provide an 'escape hatch' for applications which want long-running sessions, by allowing them to: - query the amount of elements in the cache - clear the element cache by adding two custom routes. We're looking at other approaches to keep the cache size under control as well - e.g. by checking the element cache for duplicates and adding an option to remove 'stale' elements (elements which no longer exist); we'll try to submit PRs for that as well. As usual, let me know what you think & happy to discuss further. Closes facebookarchive#896 Differential Revision: D8254682 Pulled By: marekcirkos fbshipit-source-id: a200a570d9b4e71a6f2171419b39126dd9affca3
Summary: In XCUIElementTypeTable,attribute values of many XCUIElementTypeCell instances are null.When fb_scrollToVisibleWithNormalizedScrollDistance function is called by XCUIElementTypeCell instance,it is unable to accurately slide the specified UI element. Closes facebookarchive#879 Differential Revision: D8255752 Pulled By: marekcirkos fbshipit-source-id: 930f478417823ae0c0f79d1699d61af616f3c781
Summary: Integrations tests under latest release Xcode are failing with `Unknown selector 'hitPoint'`. This is because Apple changed hitpoint interface a bit. This diff bridges 'old' & 'new' interface under one methods. By-product wins: - fixed //EndToEndTests/WebDriverAgent:IntegrationTests2 - XCElementSnapshotHitPoint/testAccessibilityActivationPoint - fixed //EndToEndTests/WebDriverAgent:IntegrationTests3 - XCUIElementFBFindTests_AttributesPage/testInvisibleDescendantWithXPathQuery Reviewed By: antiarchit Differential Revision: D8279751 fbshipit-source-id: b4ead8f459724bf7979a787ea6b1a851a1f35b59
Summary: We do not use flow annotations anymore so removing it for now. Reviewed By: antiarchit Differential Revision: D8297400 fbshipit-source-id: 791a121f879a5ae02220a4af9ff40f7c6f28076c
Summary: Make compiler happy Reviewed By: antiarchit Differential Revision: D8348679 fbshipit-source-id: 696b0e23f3b2df7cb153a359ebcb291ef58010f8
Summary: D7947931 patches taping XY for rotated screens however it does not work in case window does not cover fullscreen. In such case we need patch coordinates to take into account window's offset to correctly calculate final XY tap coordinates Reviewed By: antiarchit Differential Revision: D8820400 fbshipit-source-id: 3c526253d86b680a37fb55c25d237e4a9de459a2
Summary: More and more classes are moved from public `XCTest` framework to private `XCTAutomationSupport` one. It is not big of an issue, however in order to be able to compile WDA with new XCode we need to either: - explicitly link both frameworks (this diff) - or remove direct class allocations Direct linking would remove requirement for future refactoring that might be necessary with potentially other moved classes so going for this approach. Reviewed By: antiarchit Differential Revision: D9081652 fbshipit-source-id: 94a8162eee491ced175a56f4381b2bba9ce31c16
…882) Summary: This should improve elements lookup flow if done by native clients by returning correct webdriver response codes for incorrect locator expressions. Pull Request resolved: facebookarchive#882 Differential Revision: D9295734 Pulled By: marekcirkos fbshipit-source-id: b9ffda5f51f5f5dc7a64e7383a866152000e5357
Summary: It turns out, that `NSProcessInfo.processInfo.environment[@"USE_PORT"]` returns an empty string instead of nil if `USE_PORT` is not present in the environment which causes zero value to be assigned by default. Pull Request resolved: facebookarchive#976 Differential Revision: D9294646 Pulled By: marekcirkos fbshipit-source-id: 1501c754268f828ed922cfa978fb32ce0468cdad
Summary: Allows to forward NSKeyValueObserverRegistration methods. Fixes issue #975 — Allows to work with Xcode 10 beta 5 and beta 6. Tested for Xcode 9.4.1 as well Pull Request resolved: facebookarchive#979 Differential Revision: D9399064 Pulled By: marekcirkos fbshipit-source-id: f1976e0fdefdaaf2ed5c52eeeace6c99acb50ee8
Reviewed By: antiarchit Differential Revision: D10525731 fbshipit-source-id: 25eb57d95d8ac556104c84ccc2642fbc36a4979b
Summary: In case some object will start observing properties on `XCUIApplicationProcess`, which is proxied by `FBApplicationProcessProxy`. All KVO's callbacks will pass unexpected observed object; XCUIApplicationProcess rather than `FBApplicationProcessProxy`. So in case those objects make any checks based on that value, behaviour might be unexpected. This solution creates separate observer proxies that will proxy KVO call with proper observed object value. IT also significantly simplifies "tracing" of all observer and their observered values. Reviewed By: mkareta Differential Revision: D13432956 fbshipit-source-id: 97c8e3d22facd1dcfa77e3bb6d433120ea0fde56
Summary: We are changing `currentProcess` property in very sneaky way, and any pre-existing KVO observer on `XCUIApplicationImpl` will not get notified about it. However they should get notified, otherwise KVO will complain about unnotified object change. So this happens, because object it changeed, while already handling KVO notification. A bit hacky way is to move this out-of KVO call stack, eg by async dispatch Reviewed By: mkareta Differential Revision: D13432958 fbshipit-source-id: 015dc0dd950003a1044a9f0a850f07ea17e1fa86
|
Jon Gabilondo, Mykola Mokhnach, Anton Kushpil, Liu Junqi, 林名君 seem not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
|
Unfortunately some tests are still crashing for this PR: https://travis-ci.org/appium/WebDriverAgent/jobs/470950738#L1888 |
|
I'm pretty sure that it's because of the bug I've opened on Facebook's repository after @marekcirkos added the KVO forward mechanism to the FBApplicationProcessProxy class. While it fixes the shouldWaitForQuiescence issue there is a problem with re-activating apps (initBundleWithPrivatePath fails if there's already an XCUIApplicationProcess for it). Edit: Unfortunately it's also making createSession a bit slower (2.5 seconds before, 4.5 seconds after for opening the Settings app). |
|
@Dan-Maor Do you have any ideas on how to address it? I cannot merge these fixes in their current state, since it is going to break existing tests for application switching. |
|
|
The KVO changes are really good, but I don't think they can be safely merged at this point. I started looking into it this week but I'm afraid I don't have any solid directions yet. Looks like something with the KVO forwarding isn't clicking nicely:
Perhaps we should revert the KVO PRs until we figure it out. Unfortunately my Mac is at work so I won't be able to dig into it until next week. P.S. I had an idea for fixing shouldWaitForQuiescence but @marekcirkos's solution is more generic and I like it more, we just need to figure out what's the missing link here. |
|
Thanks for your investigation @Dan-Maor. I see the topic is quite complicated and I possibly cannot help much with that. What I can only say, that the problem that we have is not present for FB WDA, since they don't have application switching implemented. Perhaps, the current way it is done in our fork is not very optimal and needs to be improved. Please keep us updated and, again, big thanks to you and @marekcirkos for taking care about such problems. |
|
I made some progress on identifying the issue. From what I understand, it appears that while we replace XCUIApplicationProcess in the context of XCUIApplicationImpl, XCUIApplicationMonitor still has references to the old XCUIApplicationProcess objects inside internal structures (_launchedApplications, _applicationProcessesForPID and _applicationImplDepot) which link between the XCUIApplicationImpl and its equivalent XCUIApplicationProcess. It can be accessed using "[XCUIApplicationMonitor sharedMonitor]". Violently purging the sharedMonitor and its depot using KVC allows creating another session with a previously launched application, however I'd rather find a more gentle way of modifying the internal dictionaries and change the references to our newly created FBApplicationProcessProxy, if that's possible (pretty sure it is). I'm not certain about other effects of the violent approach. Might take me a while to continue with it since I'm working on other urgent tasks at the moment (I've analyzed it while waiting for a rare exception on another issue I'm working on), so if the changes are urgent I suggest to perhaps merge all but the last two commits since these are the ones in need of attention. |
|
@Dan-Maor We could easily wait, there is no rush. We'd rather merge a correct fix, which does not fail any tests. |
|
@Dan-Maor I think the problem is that dispatch_async is executed in next runloop run, but check probably takes place in the same call stack later (however proxy is already nested). We can either delay nesting proxy, but then we will miss some calls to it, or take completely different approach and replace proxy and modify |
|
@Dan-Maor Do you by any chance have time to work on this fix. BTW, have you seen |
|
Not anytime soon I’m afraid, I’m extremely overloaded at the moment. About Xcode 10.2 - from what I can tell the sharedClient has been removed in favor of a property of XCUIDevice (accessibilityClient), hopefully just using it instead will give the same behavior. This is also the case for the sharedMonitor from XCUIApplicationMonitor, by the way. |
|
Thanks for the hint @Dan-Maor |
|
After looking into it a bit more I tend to agree with @marekcirkos - I could't find a way for still using the FBApplicationProcessProxy with the KVO changes while maintaining the ability to re-activate applications. Looks like we'll have to resort to overriding some methods on runtime (I'm working on it). By the way - there's an issue currently in master where setting waitForQuiescence to false doesn't make a difference with Xcode 9.4 and above due to not registering for an observer in FBApplication if applicationProcessTracker exists (it has been added in 9.4, not 10.2). |
|
@Dan-Maor still facing this issue. The environment: Xcode 11.2.1
|
|
Are you setting shouldWaitForQuiescence to false? |
Yes, it is "NO" from WDA.
it is randomly |
|
Are you setting it explicitly to false? appium-xcuitest-driver sends it as 'true' by default unless the capability is manually set to false. |
|
@Dan-Maor Also i changed in FBSessionCommand.m but it did not work |
If the issue doesn't happen when the capability is set to true, please open an issue in the main Appium repo with all of the details.
|
|
I ran it with Xcode and set shouldWaitForQuiescence to TRUE and also FALSE. After WDA start successfully (ServerURLHere->http://172.27.168.30:8100<-ServerURLHere). I open localhost:9100, the error occurred. |
|
If setting shouldWaitForQuiescence to true doesn't make a difference then it's not the same issue as before. Opening localhost:9100 is what triggers the video stream to start, so it may be related, however mix and matching like that is never a good idea and may well be the cause of the issue. We can't really help with this sort of setup since it is not intended to run this way. |
No description provided.