-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
webdriverio: fix appium getvalue #3606
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3606 +/- ##
=========================================
Coverage ? 98.79%
=========================================
Files ? 141
Lines ? 3075
Branches ? 672
=========================================
Hits ? 3038
Misses ? 35
Partials ? 2
Continue to review full report at Codecov.
|
@@ -33,4 +33,18 @@ describe('getValue', () => { | |||
await elem.getValue() | |||
expect(request.mock.calls[2][0].uri.path).toBe('/wd/hub/session/foobar-123/element/some-elem-123/attribute/value') | |||
}) | |||
|
|||
it('should get value in mobile mode', async () => { |
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.
Does this test fail if you remove your fix for .getValue()
?
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
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.
but how? it does the same assertion as in line 34, no?
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.
@christian-bromann assertion is the same, thats correct.
However capabilities are different
capabilities: {
browserName: 'foobar', // isW3C => true
mobileMode: true // isMobile => true
}
I'm creating instance in W3C and mobile mode: mobileMode: true
if (this.isMobile) { // true, thats why test is passing currently
if (this.isW3C) { // true, thats why test will fail if I remove the fix
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.
ahh ok no I see once expand the whole file. Let's do
if (this.isW3C && !this.isMobile) {
then please. I am also wondering if this shouldn't be a bug reported in Appium.
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.
Ok, I'll update condition.
It should be either reported to Appium or if it is working as designed we need a link to prove that.
I'll take a look on Monday
@@ -33,4 +33,18 @@ describe('getValue', () => { | |||
await elem.getValue() | |||
expect(request.mock.calls[2][0].uri.path).toBe('/wd/hub/session/foobar-123/element/some-elem-123/attribute/value') | |||
}) | |||
|
|||
it('should get value in mobile mode', async () => { |
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.
but how? it does the same assertion as in line 34, no?
Appium issue logged at appium/appium#12218 |
@christian-bromann thanks a lot for help! Updating PR right now according to requested changes |
@mgrybyk don't see the change applied, did you miss to push it? |
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.
Ah no I see it , thanks 👍
Let's wait until the highlight.js bug is fixed, I will rerun and merge then |
## Proposed changes fix webdriverio#3515 use non W3C method to getValue ## Types of changes - [x] Bugfix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) ## Checklist - [x] I have read the [CONTRIBUTING](https://github.com/webdriverio/webdriverio/blob/master/CONTRIBUTING.md) doc - [x] I have added tests that prove my fix is effective or that my feature works - [ ] I have added necessary documentation (if appropriate) ## Further comments most likely we have to submit ticket to Appium if there are no such iOS ``` session/{sessionId}/element/5000/property/value Method has not yet been implemented ``` Android ``` 404 - "unknown command: session/{sessionId}/element/0.12642389298468348-1/property/value" ``` ### Reviewers: @webdriverio/technical-committee
The bug for this PR was merged and released. I am not sure which issue your are referring to here. |
@christian-bromann - I was facing the error 'Method has not yet been implemented' for 'getProperty' command with WebDriverIO + Appium. Hence wanted to verify. Thank you for the update. |
Proposed changes
fix #3515
use non W3C method to getValue
Types of changes
Checklist
Further comments
most likely we have to submit ticket to Appium if there are no such
iOS
Android
Reviewers: @webdriverio/technical-committee