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.

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

oomelianchuk
Copy link
Contributor

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!

}
catch (IOException e)
{
e.printStackTrace();

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));

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();

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))

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));

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();

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");

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";

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);

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

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.

@wurzelkuchen wurzelkuchen linked an issue May 17, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Passing Command Line Arguments To Driver
2 participants