-
Notifications
You must be signed in to change notification settings - Fork 11
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.
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
# 198 Support Passing Command Line Arguments To Driver #201
base: develop
Are you sure you want to change the base?
Conversation
} | ||
catch (IOException e) | ||
{ | ||
e.printStackTrace(); |
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.
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.
{ | ||
ImmutableList.Builder<String> argsBuilder = ImmutableList.builder(); | ||
argsBuilder.addAll(super.createArgs()); | ||
argsBuilder.add(String.format("--port=%d", 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same argument as before.
We should just throw the exception and die hard and early so it's obvious that something went wrong.
String firefoxLogFile = logPathArgs.get(0).replaceAll("--log-path=", "").trim(); | ||
if (firefoxLogFile != null) | ||
{ | ||
if ("/dev/stdout".equals(firefoxLogFile)) |
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.
are there equivalents for windows OS?
{ | ||
ImmutableList.Builder<String> argsBuilder = ImmutableList.builder(); | ||
// argsBuilder.addAll(super.createArgs()); | ||
argsBuilder.add(String.format("--port=%d", 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why was the edge path removed?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Issue has a list with possible arguments, but the issue is quite old, please check if everything is still valid for current browsers.
@RunWith(NeodymiumRunner.class) | ||
@Browser("Chrome_headless") | ||
@Browser("FF_headless") | ||
public class DriverArgumentsTest extends NeodymiumTest |
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.
please add a test for an empty config string as well.
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!