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

Make all XCTest gestures available for "mobile:" interface #393

Merged
merged 1 commit into from
Mar 16, 2017

Conversation

mykola-mokhnach
Copy link
Contributor

This PR makes all XCTest gestures available for calling via mobile: interface, so one can use them even without specialised wrappers in top-level drivers. This is just a POC, so I didn't create any tests yet. Please kindly check the basic implementation first.

@mykola-mokhnach
Copy link
Contributor Author

@sergey-plevako-badoo this is not exactly what you wanted to implement in #170, but should work as well.

@mykola-mokhnach mykola-mokhnach changed the title Make all XCTest gestures available from mobile: interface Make all XCTest gestures available for "mobile:" interface Mar 14, 2017
Copy link
Member

@jlipps jlipps left a comment

Choose a reason for hiding this comment

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

a style comment but otherwise looks good. sad, but good :-)

scale: parseFloatParameter('scale', opts.scale, 'pinch'),
velocity: parseFloatParameter('velocity', opts.velocity, 'pinch')
};
const el = opts.element.ELEMENT ? opts.element.ELEMENT : opts.element;
Copy link
Member

Choose a reason for hiding this comment

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

this pattern can be more succinctly written as:

const el = opts.element.ELEMENT || opts.element;

@mykola-mokhnach mykola-mokhnach force-pushed the gestures branch 2 times, most recently from a739b24 to 4753d3a Compare March 15, 2017 09:02
@mykola-mokhnach
Copy link
Contributor Author

Fixed review comments and squashed the stuff

Copy link
Contributor

@imurchie imurchie left a comment

Choose a reason for hiding this comment

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

Seems good. Tests would be nice, at some point.

Copy link
Member

@jlipps jlipps left a comment

Choose a reason for hiding this comment

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

LGTM now, one question about return

}
await mobileCommandsMapping[mobileCommand](opts);
Copy link
Member

Choose a reason for hiding this comment

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

wonder whether this actually needs to be return await in order to ensure that if WDA has any kind of return value it is passed back to the client?

Copy link
Member

Choose a reason for hiding this comment

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

I know the current behavior also doesn't have return but it seems like that could be missing. however none of the proxy commands have return so we'd have to make this change everywhere if we cared about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

piece of cake :)
Added return

velocity: parseFloatParameter('velocity', opts.velocity, 'pinch')
};
const el = opts.element.ELEMENT || opts.element;
await this.proxyCommand(`/wda/element/${el}/pinch`, 'POST', params);
Copy link
Member

Choose a reason for hiding this comment

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

awesome, however what i was subsequently reflecting on is that we'd need to have a return here, and in each other instance of proxyCommand inside a mobile method as well, if we want the outer return you just added to mean anything :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

still simple ;)

Copy link
Member

@jlipps jlipps left a comment

Choose a reason for hiding this comment

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

you are a gentlehuman and a scholar 🎩

@mykola-mokhnach
Copy link
Contributor Author

34006429

Thanks for the good words. Just trying not only to take but also to share something to the community.

@jlipps
Copy link
Member

jlipps commented Mar 15, 2017

That's the spirit! 🙇 May we find some way to cultivate more contributors like you :-)

@imurchie
Copy link
Contributor

Looks good!

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.

3 participants