Skip to content
This repository has been archived by the owner on Dec 18, 2019. It is now read-only.

Detect port #30

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

crw2998
Copy link

@crw2998 crw2998 commented Sep 12, 2017

The other pull request was on the wrong branch :)

This change would allow wdio-selenium-standalone-service to detect a custom port defined in the wdio config file for the Selenium standalone server. This would be helpful for people running multiple instances of a test suite concurrently on the same machine, allowing them to pass in the desired port number only to the wdio command, and not have to also specify it in seleniumArgs in the config file. A custom port for the standalone server is necessary to prevent conflicts between different testing instances.

@crw2998
Copy link
Author

crw2998 commented Sep 12, 2017

I suppose there needs to be a flag specifying if you want the port copied.

Copy link
Contributor

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

@crw2998 do you think we can add a unit test for this?

@crw2998
Copy link
Author

crw2998 commented Sep 12, 2017

Sure. I can update the readme as well with some info, if that makes sense.

@christian-bromann
Copy link
Contributor

@crw2998 that would be awesome!

README.md Outdated
For example:
```js
port: 4441,
usePortInStandalone: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why adding this extra option? Can't we just always apply the port number (or everytime when it is not 4444)?

Copy link
Author

Choose a reason for hiding this comment

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

I thought at first this was why the CI build was failing (maybe it was looking for the server being up on a specific port), but now I see that's not the case. I guess I'm not sure in what situation you would want the standalone server to be running on a different port than the tests, so I'll take it off.

Copy link
Contributor

Choose a reason for hiding this comment

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

tests are failing because Array.includes is not supported with the node version running in CI/CD

lib/launcher.js Outdated
if (this.seleniumArgs.seleniumArgs === undefined) {
this.seleniumArgs.seleniumArgs = []
}
if (!this.seleniumArgs.seleniumArgs.includes('-port')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's not use includes because it is not supported in node lower than 6 I think

@christian-bromann
Copy link
Contributor

@crw2998 tests are still failing due to connection issues. I think it is related to the change

@crw2998
Copy link
Author

crw2998 commented Sep 13, 2017

Hmm. If I clone the master branch and run the tests locally, the test fails even without my code, which is why I was ignoring the test failure. I see the same errors on this PR as on the master branch, but in both of my local runs, the browser instance launches but I get a failed to exec test script which looks like a different problem than the connection issue.

Also, if I put my PR build into my own WebdriverIO project, it works with and without a custom port (and actually launches on the correct port). I'm not sure what it is in the test configuration that is causing the failure.

@christian-bromann
Copy link
Contributor

@crw2998 I just ran a build from master and it passed: https://travis-ci.org/webdriverio/wdio-selenium-standalone-service/builds/250575620 .. so it seems it definitely is connected to the change

@crw2998
Copy link
Author

crw2998 commented Sep 13, 2017

Okay, I'll take a look.

@Miladinho
Copy link

@christian-bromann

My pull request today just changes the README and tests are still failing...

@aberonni
Copy link
Contributor

aberonni commented Jan 5, 2018

@christian-bromann @Miladinho @crw2998 I submitted #38 to try and fix the issues with travis. Maybe watch that PR and rebase if it seems ok and gets onto master.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants