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

stop chromedriver proxies when reset command is called. fixes #12083 #495

Merged
merged 1 commit into from
Feb 5, 2019
Merged

stop chromedriver proxies when reset command is called. fixes #12083 #495

merged 1 commit into from
Feb 5, 2019

Conversation

Jonahss
Copy link
Member

@Jonahss Jonahss commented Jan 29, 2019

The same exact command is already being called on closeApp: https://github.com/appium/appium-android-driver/blob/master/lib/commands/general.js#L262

A side note: should both of these commands respect the recreateChromeDriverSessions desired capability? Then we should call suspendChromedriverProxy instead of stopChromedriverProxies like here:
https://github.com/appium/appium-android-driver/blob/master/lib/commands/context.js#L71

@jsf-clabot
Copy link

jsf-clabot commented Jan 29, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@Jonahss
Copy link
Member Author

Jonahss commented Jan 29, 2019

Oh, and should I add an e2e test for this case?

@Jonahss
Copy link
Member Author

Jonahss commented Jan 29, 2019

Fixes #495

@Jonahss Jonahss mentioned this pull request Jan 29, 2019
Closed
KazuCocoa
KazuCocoa reviewed Jan 30, 2019
View reviewed changes
lib/commands/general.js Outdated
@@ -228,7 +228,7 @@ commands.reset = async function () {
await androidHelpers.resetApp(this.adb, Object.assign({}, this.opts, {fastReset: true}));
// reset context since we don't know what kind on context we will end up after app launch.
this.curContext = NATIVE_WIN;

await this.stopChromedriverProxies();
Copy link
Member

Choose a reason for hiding this comment

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

What about calling switchContext(NATIVE_WIN)?
As you mentioned in your side note, if we set native window, the method calls stop or suspend chrome driver properly..?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I was just following the pattern from closeApp in the same file. If we call switchContect we get the added benefit of respecting the recreateChromeDriverSessions desired cap.
On closeApp are we ok leaving chromedriver still running if somebody set this cap?

@Jonahss Jonahss force-pushed the stop-chromedriver-on-reset branch from 133ea74 to cb5fa9e Compare January 30, 2019 21:17
@Jonahss
Copy link
Member Author

Jonahss commented Jan 30, 2019

@KazuCocoa just pushed another try at this.
switchContext() didn't work because if fails if you try to switch to the context that's already the current context. setContext() does this check, so called that.

setContext compares the context you pass in to this.curContext. The issue I ran into is that the unit tests don't actually call the createSession method which sets curContext = this.defaultContext(). I moved the initial setting to the defaultContext into the constructor. I thought this was OK because it's not clear if there is ever a case where this.curContext makes sense to be undefined.

KazuCocoa
KazuCocoa approved these changes Jan 31, 2019
View reviewed changes
Copy link
Member

@KazuCocoa KazuCocoa left a comment

Choose a reason for hiding this comment

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

I left one comment. Otherwise, LGTM 👍

lib/commands/context.js Outdated
@@ -37,7 +37,7 @@ commands.getContexts = async function () {
};

commands.setContext = async function (name) {
if (name === null) {
if (name === null || name === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

what about #hasValue by import { util } from 'appium-support';?
https://github.com/appium/appium-support/blob/2fcf470576f0357e3200f8014f8b1898b326d79e/lib/util.js#L14

Copy link
Member Author

Choose a reason for hiding this comment

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

ooh that's much better. I love that.

@Jonahss Jonahss force-pushed the stop-chromedriver-on-reset branch from cb5fa9e to d773c11 Compare February 1, 2019 00:11
@Jonahss
Copy link
Member Author

Jonahss commented Feb 1, 2019

@KazuCocoa PR updated with suggestion for using hasValue()

KazuCocoa
KazuCocoa approved these changes Feb 1, 2019
View reviewed changes
Copy link
Member

@KazuCocoa KazuCocoa left a comment

Choose a reason for hiding this comment

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

👍

@Jonahss
Copy link
Member Author

Jonahss commented Feb 4, 2019

CI tests failed, so I want to take a look before merging. Looks related.

@Jonahss Jonahss force-pushed the stop-chromedriver-on-reset branch from d773c11 to 0b66aad Compare February 4, 2019 21:16
@Jonahss Jonahss force-pushed the stop-chromedriver-on-reset branch from 0b66aad to 0d75041 Compare February 4, 2019 21:52
@Jonahss Jonahss merged commit fb3dfd7 into appium:master Feb 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KazuCocoa KazuCocoa KazuCocoa approved these changes

@jlipps jlipps jlipps 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.
4 participants
@Jonahss @jsf-clabot @jlipps @KazuCocoa