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.

Sign up for GitHub

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

Jump to bottom

[java][sm] Configure Selenium Manager environment from System Properties #13858

Merged
merged 3 commits into from May 6, 2024
Merged

[java][sm] Configure Selenium Manager environment from System Properties #13858

merged 3 commits into from May 6, 2024

Conversation

kool79
Copy link
Contributor

@kool79 kool79 commented Apr 23, 2024 •
edited by codiumai-pr-agent-pro bot

User description

Fixes #13857

Allow to configure Selenium Manager from java bindings with help of java System Properties

Description

Configure a Selenium Manager with a system properties from Java

When create process for Selenium Manager, pass the System Properties from java, which start with SE_ prefix, to the Selenium Manager process. With this approach, non-empty system properties, which are defined in java, have a higher priority over the environment variables (for Selenium Manager process only)

Motivation and Context

See #13857

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Type

Enhancement

Description

  • This PR introduces the ability to configure the Selenium Manager using Java system properties that start with the SE_ prefix.
  • System properties are now prioritized over environment variables for configuring the Selenium Manager, providing more flexibility and control from within Java applications.
  • The process builder for the Selenium Manager has been updated to include these system properties in its environment, ensuring that configurations are respected during runtime.

Changes walkthrough

Relevant files
Enhancement
SeleniumManager.java
Enhance Selenium Manager Configuration via System Properties

java/src/org/openqa/selenium/manager/SeleniumManager.java

  • Added support for configuring Selenium Manager using Java system
    properties prefixed with SE_.
  • Enhanced the process builder to include system properties in the
    environment of the Selenium Manager process.
  • Modified cache path determination logic to prioritize system
    properties over environment variables.
    		•  +23/-7   

    ✨ PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @CLAassistant
    Copy link

    CLAassistant commented Apr 23, 2024 •
    edited

    CLA assistant check
    All committers have signed the CLA.

    @codiumai-pr-agent-pro CodiumAI PR-Agent Pro
    Copy link

    PR Description updated to latest commit (25964eb)

    @codiumai-pr-agent-pro CodiumAI PR-Agent Pro
    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves changes to the environment configuration logic which can have wide-ranging effects. Understanding and verifying the new logic for prioritizing system properties over environment variables, and ensuring it doesn't introduce regressions or unexpected behavior, requires a thorough review.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The new system property handling logic might introduce concurrency issues if properties are changed by other parts of the application during the iteration. The comment about reading property with 'default' value due to concurrency might not be sufficient to prevent issues.

    Possible Bug: The change in cache path logic might cause issues if the system property or environment variable is not set, as it now defaults to an empty string which could lead to incorrect path computations.

    🔒 Security concerns

    No

    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

    [pr_reviewer]
some_config1=...
some_config2=...

    See the review usage page for a comprehensive guide on using this tool.

    @codiumai-pr-agent-pro CodiumAI PR-Agent Pro
    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Improve the fetching of environment-specific properties by using environment variables directly.

    Replace the use of System.getProperties() with System.getenv() for consistency and to
    avoid potential issues with properties not being set as environment variables. This change
    ensures that the environment-specific properties are correctly fetched from the
    environment variables, which is typically the expected source in deployment environments.

    java/src/org/openqa/selenium/manager/SeleniumManager.java [126-133]

    -Properties properties = System.getProperties();
-for (String name : properties.stringPropertyNames()) {
+Map<String, String> env = System.getenv();
+for (String name : env.keySet()) {
     if (name.startsWith(SE_ENV_PREFIX)) {
-        String value = properties.getProperty(name, "");
-        if (!value.isEmpty())
+        String value = env.get(name);
+        if (value != null && !value.isEmpty())
             processBuilder.environment(name, value);
     }
 }
    Enhance thread safety by using concurrent collections for environment variables.

    To ensure thread safety and avoid potential race conditions with mutable static fields,
    consider using ConcurrentHashMap for environment variables in processBuilder.

    java/src/org/openqa/selenium/manager/SeleniumManager.java [132]

    -processBuilder.environment(name, value);
+processBuilder.environment().putIfAbsent(name, value);
    Bug
    Prevent potential NullPointerException by ensuring cachePath is not null before modification.

    Add a null check for cachePath before attempting to replace the HOME placeholder to
    prevent potential NullPointerException.

    java/src/org/openqa/selenium/manager/SeleniumManager.java [265]

    -cachePath = cachePath.replace(HOME, System.getProperty("user.home"));
+if (cachePath != null) {
+    cachePath = cachePath.replace(HOME, System.getProperty("user.home"));
+}
    Maintainability
    Improve command construction readability and maintainability in process builder.

    Use String.join to construct the command for processBuilder to enhance readability and
    maintainability.

    java/src/org/openqa/selenium/manager/SeleniumManager.java [136]

    -processBuilder.command(binary.toAbsolutePath().toString(), arguments).start();
+processBuilder.command(String.join(" ", binary.toAbsolutePath().toString(), String.join(" ", arguments))).start();
    Best practice
    Use a more specific exception for clarity in error handling related to environment configuration.

    Consider using a more specific exception than IOException for errors related to
    environment variable configuration to provide clearer error handling.

    java/src/org/openqa/selenium/manager/SeleniumManager.java [256]

    -private Path getBinaryInCache(String binaryName) throws IOException {
+private Path getBinaryInCache(String binaryName) throws ConfigurationException {
    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

    [pr_code_suggestions]
some_config1=...
some_config2=...

    See the improve usage page for a comprehensive guide on using this tool.

    @diemol diemol added this to the 4.21 milestone Apr 23, 2024
    diemol
    diemol approved these changes May 6, 2024
    View reviewed changes
    Copy link
    Member

    @diemol diemol left a comment

    Choose a reason for hiding this comment

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

    Thank you, @kool79!

    @diemol
    Copy link
    Member

    diemol commented May 6, 2024

    @kool79 do you think you can help us and update the docs for Selenium Manager, please?

    @diemol diemol merged commit 7b83fc1 into SeleniumHQ:trunk May 6, 2024
    39 of 40 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @diemol diemol diemol approved these changes

    Assignees
    No one assigned
    Labels
    enhancement Review effort [1-5]: 3
    Projects
    None yet
    Milestone
    4.21
    Development

    Successfully merging this pull request may close these issues.

    [🚀 Feature]: Allow to configure Selenium Manager from java
    3 participants
    @kool79 @CLAassistant @diemol