-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
feat: Add findByImage support to Windows driver #67
Conversation
Wow - such a quick response! Thank you! This is a big blocker for me at the moment with what I am trying to do, so much appreciated! |
@jsa34 Could you please check out this code and test it locally? With your help it would go faster |
Of course! Let me see... |
Trying by npm is being difficult, won't let me install git repo package, just stuck at rollbackFailedOptional... |
You can simply install appium beta and then replace or link (see |
Probably just me as I am much more of a Java kind of guy, but using windows here and npm link just keeps giving me "Access is denied" response, but nothing seems weird with permissions that I can see... |
appium/appium#13227 (comment) contains more detailed tutorial. Simply replace occurrences of |
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.
🎉
The implementation looks good to me.
(Haven't tested on WinAppDriver though)
Still seem to get the same response, but think I've followed the above guide correctly: Code:
Error in IDE: Appium:
|
This is easy to check, simply add something like |
Yep, as expected, it isn't calling find.js. I had noticed this before, though, and it may be the root cause of the problem: No matter what I do, all .findElement calls, etc, return back an element of type WebElement, and I can never get a WindowsElement. In the error stacktrace, it looks like appium is resolving to the native selenium find methods so never looks for find.js...
|
Just trying with a simple driver.findElementByImage results in the same, all the findElementBy* return WebElements instead of WindowsElements for the following simple code (Tried to make sure at each step drivers are definitely casted to WindowsDriver and elements are casted to WindowsElements if some reason they aren't) ... (Guessing this is a java-client issue?)
Error:
|
Are you sure you use appium@beta as the base? With 1.16 it won't work for sure |
Yes: [Appium] Welcome to Appium v1.17.0-beta.1 (REV 8cffb5274854c4cc1e639d56fb2e539c6a5c1786) Is logged on startup |
ok, this is fine. Now it might necessary to check if the endpoint is proxied properly. For that you might add some logging to the appium-base-driver module:
|
the simplest way to add logging would be to just modify the corresponding files inside |
|
||
const commands = {}; | ||
|
||
commands.findElOrEls = async function findElOrEls (strategy, selector, mult, context) { |
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.
seems like we could have a generic findElOrElsByProxy
method since we do this in many drivers.?
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.
yes, this improvement could be done in a separate PR
Managed to find the problem - it was silently failing to update the build after doing an install... Fixed now. So, I have 2 issues:
UPDATE: Discovered this is because the driver was instantiated at root level, which apparently throws a null pointer for .manage().window().getSize(). After creating a driver linked to a window handle, it no longer throws null pointers and there is no issue. Perhaps a helpful error message would help here? 2.Previously working locators now fail: I tried the RegExs before the last commit you made, and thought it was worth noting that with mykola-mokhnach@f4989e6 commit reversed, the old behaviour returns: all locators other than image are fine, and image throws a "InvalidSelectorError" |
Cool, this is already something. Let me do some more magic... |
After 84570b7 the errors above should be gone ;) |
Fab - all locators are now functional! Tried finding by image, but getting the following error (made sure it has started a driver session with a top level window handle defined):
Example of driver creation code:
Also tried d.manage().window().getSize() and this returned a null pointer... Suspect this is the root cause |
This API is expected to return Could you please describe this issue to Microsoft and see what their response would be? |
Btw, they declare the I would also make sure we use the most recent server version from them |
Hello - I am using 1.2-RC... I can't see why this is returning as I too saw it listed in their supported endpoints... |
Is there a way of trying different winappdriver versions, as using 1.7.0 beta seems to tie me to 1.2-RC version so I can't verify if other versions of winappdriver support this endpoint? |
No worries - found how to do it... Just trying different winappdriver versions... |
So, tried the following, but no luck:
I have filed an issue with WAD team, but they don't seem to maintaining it from recent activity... |
Just seen a possible workaround would be to use the element of the window instead of the /window/size endpoint as they should be equal according to the test: Not sure if this also returns null or if it is a viable workaround, but just thought it worth mentioning |
I've added a workaround there. I don't like it, but that's anyway better than waiting until MS fixes the server. Please try it and let me know if it works |
Great! This is working now! |
Just ran it again, not sure what I changed, but MobileBy throws the following...
And trying to not use MobileBy and a native findsByImage throws casting exception: Using the following code (DriverManager.getDriver() already returns an instance of WindowsDriver but I am trying to cover all casting issues...):
|
These errors are expected. We'd have to add a wrapper for touch actions to make clicks working same way the wrappers for screenshots/screensize have been added, but not in this PR. |
} | ||
// workaround for https://github.com/microsoft/WinAppDriver/issues/1104 | ||
log.info('Cannot retrieve window size from WinAppDriver. ' + | ||
'Falling back to WMIC usage'); |
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.
wow...ah..
Thanks for the explanation - I will have a look at the wrapper solution for touch actions, but I still don't understand why: |
appium/appium#14073