Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Support for log_event and get_events command#469

Merged
ki4070ma merged 15 commits intoappium:masterappium/python-client:masterfrom
ki4070ma:fix-eventsCopy head branch name to clipboard
Nov 18, 2019
Merged

Support for log_event and get_events command#469
ki4070ma merged 15 commits intoappium:masterappium/python-client:masterfrom
ki4070ma:fix-eventsCopy head branch name to clipboard

Conversation

@ki4070ma
Copy link
Collaborator

@ki4070ma ki4070ma commented Nov 9, 2019

appium/webdriver/webdriver.py Outdated Show resolved Hide resolved
appium/webdriver/webdriver.py Outdated Show resolved Hide resolved
@ki4070ma ki4070ma changed the title Use appium/events as endpoint to get events [WIP] Use appium/events as endpoint to get events Nov 9, 2019
@ki4070ma ki4070ma changed the title [WIP] Use appium/events as endpoint to get events Support for log_event and event command Nov 10, 2019
appium/webdriver/extensions/log_event.py Outdated Show resolved Hide resolved
appium/webdriver/extensions/log_event.py Outdated Show resolved Hide resolved
appium/webdriver/extensions/log_event.py Outdated Show resolved Hide resolved
appium/webdriver/extensions/log_event.py Outdated Show resolved Hide resolved
@mykola-mokhnach
Copy link
Contributor

cc @dpgraham

Can you please take a look at this change? I remember you were working on #429

@mykola-mokhnach
Copy link
Contributor

I am ok with the PR, although I would like @dpgraham to check it before merging, since it might be a breaking one

@ki4070ma ki4070ma requested a review from dpgraham November 10, 2019 12:17
class LogEvent(webdriver.Remote):

@property
def events(self):
Copy link
Member

@KazuCocoa KazuCocoa Nov 10, 2019

Choose a reason for hiding this comment

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

https://github.com/appium/appium-base-driver/blob/master/lib/protocol/routes.js#L589-L594
/events route accepts type argument as option.
It hasn't been implemented yet, but this events should not be property.

You can rename here as get_events(self, type = None) to keep another events as session_capability['events']. It is for backword compatibility as #469 (comment)

Copy link
Collaborator Author

@ki4070ma ki4070ma Nov 10, 2019

Choose a reason for hiding this comment

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

@KazuCocoa
Thanks, I misunderstood, I will keep original events and create get_events as another one.

@ki4070ma ki4070ma changed the title Support for log_event and event command Support for log_event and get_events command Nov 10, 2019
appium/webdriver/webdriver.py Show resolved Hide resolved
It isn't implemented yet for now
startTime: (int) Received time
endTime: (init) Response time
"""
return self.execute(Command.GET_EVENTS)['value']
Copy link
Member

Choose a reason for hiding this comment

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

As a client, below two cases are available

self.execute(Command.LOG_EVENT, {})
self.execute(Command.LOG_EVENT, {'type': nil}) (<= Appium server will handle this 'type' to filter events)

Copy link
Collaborator Author

@ki4070ma ki4070ma Nov 10, 2019

Choose a reason for hiding this comment

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

Yes, I've checked and added, but removed by below commit since it isn't implemented yet.
In my opinion, it's enough to add type when it's ready.

0a84069

However, I'll restore type along to your comments. Any other comments are welcome.

Copy link
Member

Choose a reason for hiding this comment

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

It could depend on server-side if the parameter works.
We can handle the behaviour only server-side modification (and documentation on appium.io), if clients follow the route definition like other commands.

The value isn't passed to the server for now.
@KazuCocoa
Copy link
Member

Added type.
Please check the description in appium/appium-base-driver#368 about the available format

@ki4070ma ki4070ma requested a review from KazuCocoa November 16, 2019 15:09
@ki4070ma ki4070ma merged commit b797254 into appium:master Nov 18, 2019
@ki4070ma ki4070ma deleted the fix-events branch November 18, 2019 14:51
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

Morty Proxy This is a proxified and sanitized view of the page, visit original site.