# 198 Support Passing Command Line Arguments To Driver #201
Conversation
| } | ||
| catch (IOException e) | ||
| { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
This might cause much confusion. If there's an IO error (for example due to rights issues or something), this method will return null, the calling method however will be just checking for null and ignore all given args. So someone would need to check the - hopefully somewhere logged - system log, which might be inside a docker or something else to check what happened and why the given args are not used.
We should just throw the exception and die hard and early so it's obvious that something went wrong.
There was a problem hiding this comment.
Removed during refactoring
| { | ||
| ImmutableList.Builder<String> argsBuilder = ImmutableList.builder(); | ||
| argsBuilder.addAll(super.createArgs()); | ||
| argsBuilder.add(String.format("--port=%d", port)); |
There was a problem hiding this comment.
Why do we set a random port here. What happens if the user is setting "---port" via args? Shouldn't this be checked before we add a random port?
| } | ||
| catch (IOException e) | ||
| { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Same argument as before.
We should just throw the exception and die hard and early so it's obvious that something went wrong.
There was a problem hiding this comment.
Removed during refactoring
There was a problem hiding this comment.
Removed during refactoring
| String firefoxLogFile = logPathArgs.get(0).replaceAll("--log-path=", "").trim(); | ||
| if (firefoxLogFile != null) | ||
| { | ||
| if ("/dev/stdout".equals(firefoxLogFile)) |
There was a problem hiding this comment.
are there equivalents for windows OS?
There was a problem hiding this comment.
after refactoring delegated to Selenium (the "/dev/stdout" and "/dev/stderr", etc. are cross-platform in Selenium, these are only indicators for where to send logs output, in the end it's done via System.out or System.err)
| { | ||
| ImmutableList.Builder<String> argsBuilder = ImmutableList.builder(); | ||
| // argsBuilder.addAll(super.createArgs()); | ||
| argsBuilder.add(String.format("--port=%d", port)); |
There was a problem hiding this comment.
Same question as before, we should probably handle the case that the user wanted to manually override the port.
| @@ -242,15 +242,28 @@ public interface NeodymiumConfiguration extends Mutable | |||
| @Key("neodymium.webDriver.chrome.pathToDriverServer") | |||
| public String getChromeDriverPath(); | |||
|
|
|||
| @Key("neodymium.webDriver.edge.pathToDriverServer") | |||
| public String getEdgeDriverPath(); | |||
There was a problem hiding this comment.
why was the edge path removed?
There was a problem hiding this comment.
thanks for pointing this out! It should not be removed, furthermore, it missed implementation as IE and Edge now require different setups. Setup for Edge implemented. The setup for IE stays the same but it's now in investigation if we still need to support IE
| Map<String, String> properties1 = new HashMap<>(); | ||
| properties1.put("neodymium.webDriver.chrome.driverArguments", "--silent ; --log-path=" + randomLogFileName); | ||
| properties1.put("neodymium.webDriver.firefox.driverArguments", "--log ; fatal ; --log-path=" + randomLogFileName); | ||
| File tempConfigFile1 = new File("./config/dev-neodymium.properties"); |
There was a problem hiding this comment.
please add a handling to not get the dev-neodymium.properties removed. See (NeodymiumContextTest.java as example.
| @Browser("FF_headless") | ||
| public class DriverArgumentsTest extends NeodymiumTest | ||
| { | ||
| private static String randomLogFileName = "target/" + UUID.randomUUID().toString() + ".log"; |
There was a problem hiding this comment.
Sine ports were handled specifically please add a unit test to check what happens if the user adds a port manually.
| public static void createSettings() | ||
| { | ||
| Map<String, String> properties1 = new HashMap<>(); | ||
| properties1.put("neodymium.webDriver.chrome.driverArguments", "--silent ; --log-path=" + randomLogFileName); |
There was a problem hiding this comment.
The Issue has a list with possible arguments, but the issue is quite old, please check if everything is still valid for current browsers.
There was a problem hiding this comment.
It seems like the list for geckodriver arguments is now a bit bigger :) It should not be a problem though, as we are, mostly, not parsing the args, but simply passing them to the driver. The user should know better what args can be passed. The best way to check, what args are supported by the current driver version is to do " --help"
| @RunWith(NeodymiumRunner.class) | ||
| @Browser("Chrome_headless") | ||
| @Browser("FF_headless") | ||
| public class DriverArgumentsTest extends NeodymiumTest |
There was a problem hiding this comment.
please add a test for an empty config string as well.
There was a problem hiding this comment.
implicity covered with other tests, adding tests for invalid values will test browser behavior but not our code
support-driver-arguments
oomelianchuk
requested review from
wurzelkuchen
and removed request for
occupant23
Added driver arguments support for Chrome, Firefox, IE and Safari (Opera and PhantomJs won't be supported by Selenium in the next version, so the is no point to develop a feature for those browsers). As the most popular and, what's more important, cross-platform browsers are Chrome and Firefox, the solution was tested for these browsers, both manually and via unit tests.
PS: It would be good to know, that the solution also works for ie and safari, so feel free to try the feature out on your platform, especially if you have access to safari and ie browsers. Thanks!