Detect port #30
base: master
Are you sure you want to change the base?
Detect port #30
Conversation
I suppose there needs to be a flag specifying if you want the port copied. |
There was a problem hiding this 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?
Sure. I can update the readme as well with some info, if that makes sense. |
@crw2998 that would be awesome! |
README.md
Outdated
For example: | ||
```js | ||
port: 4441, | ||
usePortInStandalone: true, |
There was a problem hiding this comment.
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
)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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')) { |
There was a problem hiding this comment.
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
@crw2998 tests are still failing due to connection issues. I think it is related to the change |
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 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. |
@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 |
Okay, I'll take a look. |
My pull request today just changes the README and tests are still failing... |
@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. |
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.