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.

Sign up for GitHub

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

Jump to bottom

Implemented driver status check. #1153

Merged
merged 6 commits into from
Jun 21, 2019
Merged

Implemented driver status check. #1153

merged 6 commits into from
Jun 21, 2019

Conversation

anilkrverma
Copy link
Contributor

@anilkrverma anilkrverma commented Jun 13, 2019 •
edited
Loading

Change list

Feature: Add support for "driver.status()" - #1137

Types of changes

What types of changes are you proposing/introducing to Java client?
Put an x in the boxes that apply

  • [x ] No changes in production code.
  • Bugfix (non-breaking change which fixes an issue)
  • [x ] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Details

  • Implemented driver status check as requested in Feature: Add support for "driver.status()" #1137 which returns AppiumServerStatus object containing a field for returning the version of running appium server.
  • Added a sample test to validate if the version is of form "x.y"

@anilkrverma
Copy link
Contributor Author

The tests are failing even when no changes are made in the code.
Refer #1155 which has a change in Readme text only.

This failure shouldn't be related to the code implemented here.

jlipps
jlipps reviewed Jun 13, 2019
View reviewed changes
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.

👍 @SrinivasanTarget what do you think?

anilkrverma
anilkrverma commented Jun 16, 2019
View reviewed changes
Copy link
Contributor Author

@anilkrverma anilkrverma left a comment

Choose a reason for hiding this comment

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

@jlipps @mykola-mokhnach @saikrishna321 @SrinivasanTarget Please review and merge if it looks good.
We need to fix breaking CI tests separately.

jlipps
jlipps approved these changes Jun 16, 2019
View reviewed changes
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.

this looks ok to me though it'd be nice to get the changes without a bunch of additional formatting changes as well (unless @SrinivasanTarget thinks they are a good idea too)

SrinivasanTarget
SrinivasanTarget requested changes Jun 16, 2019
View reviewed changes
Copy link
Member

@SrinivasanTarget SrinivasanTarget left a comment

Choose a reason for hiding this comment

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

Code looks good now. Can you keep styling as per the style guides we use?

@anilkrverma
Copy link
Contributor Author

Code looks good now. Can you keep styling as per the style guides we use?

@SrinivasanTarget Current changes have been done through standard auto-formatting in Java. Any reference to style guides doc for my understanding so that I can update it accordingly and be proactive in my future commits?

@SrinivasanTarget
Copy link
Member

SrinivasanTarget commented Jun 16, 2019 •
edited
Loading

@anilkrverma https://github.com/appium/java-client/blob/master/google-style.xml
Gradle task: ./gradlew clean checkstyleMain

@anilkrverma
Copy link
Contributor Author

@anilkrverma https://github.com/appium/java-client/blob/master/google-style.xml
Gradle task: ./gradlew clean checkstyleMain

@SrinivasanTarget Pushed the changes after fixing the violations reported. Please review.

@SrinivasanTarget
Copy link
Member

@anilkrverma Can you rebase your branch with the latest master once as a final nit. CI should be green after that.

@anilkrverma
Copy link
Contributor Author

Hurray, CI is green now. :)

@anilkrverma
Copy link
Contributor Author

@SrinivasanTarget , @mykola-mokhnach Rebased from master. Please review.

@anilkrverma
Copy link
Contributor Author

@SrinivasanTarget @mykola-mokhnach @jlipps @saikrishna321 We are getting below error again:
Error: /Users/vsts/agent/2.153.2/work/1/s/gradlew failed with return code: 1

It was working before, Did we broke Azure CI again or am I missing something here?

@mykola-mokhnach
Copy link
Contributor

This does not look like a rebase

@anilkrverma
- Implemented driver status check.
b42cee8 
- Solves Issue: appium#1137
@anilkrverma
- Made version field private.
1b6297c
@anilkrverma
- Changed the getStatus() method implementation to return versionMap …
71eebcb 
…instead of AppiumServerStatus class object.

- Deleted AppiumServerStatus class as it is no longer needed.
@anilkrverma
Fixed below checkstyle violations:
0583b51 
- Empty line should be followed by <p> tag on the next line.	234
- First sentence of Javadoc is missing an ending period.	234

Ran ./gradlew clean checkstyleMain, BUILD is SUCCESSFUL now.
@anilkrverma
- Updated getStatus() method to return statusMap.
1077885
@anilkrverma
Copy link
Contributor Author

This does not look like a rebase
Should be good now. @mykola-mokhnach

anilkrverma
anilkrverma commented Jun 21, 2019
View reviewed changes
Copy link
Contributor Author

@anilkrverma anilkrverma left a comment

Choose a reason for hiding this comment

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

@mykola-mokhnach @saikrishna321 @SrinivasanTarget @jlipps Hope this looks perfect now for getting merged with master. Please let me know if further changes are required.

@saikrishna321
Copy link
Member

Thanks for addressing all the comments. Looks good👍

@saikrishna321 saikrishna321 merged commit 3249bd6 into appium:master Jun 21, 2019
@jlipps
Copy link
Member

jlipps commented Jun 21, 2019

🎉 congrats @anilkrverma!

@anilkrverma
Copy link
Contributor Author

Thanks @jlipps , @saikrishna321 @SrinivasanTarget @mykola-mokhnach for all the support.

@takeyaqa takeyaqa mentioned this pull request Jul 8, 2019
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SrinivasanTarget SrinivasanTarget SrinivasanTarget approved these changes

@jlipps jlipps jlipps approved these changes

@saikrishna321 saikrishna321 saikrishna321 approved these changes

@mykola-mokhnach mykola-mokhnach mykola-mokhnach approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@anilkrverma @SrinivasanTarget @mykola-mokhnach @saikrishna321 @jlipps