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

[java][grid]: Set test name to video file name in dynamic grid #13907

Merged
merged 3 commits into from
May 6, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented May 5, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

[java][grid]: Set test name to video file name in dynamic grid

Motivation and Context

This is based on the support of Metadata in tests. When running test with dynamic grid, by adding metadata to your tests, the video file name will extract the value of capability se:name and then set to output the video file name.

For example, in Python binding:

from selenium.webdriver.chrome.options import Options as ChromeOptions
from selenium import webdriver

options = ChromeOptions()
options.set_capability('se:recordVideo', True)
options.set_capability('se:screenResolution', '1920x1080')
options.set_capability('se:name', 'test_visit_basic_auth_secured_page (ChromeTests)')
driver = webdriver.Remote(options=options, command_executor="http://localhost:4444")
driver.get("https://selenium.dev")
driver.quit()

After test executed, under /assets you can see the video file name under /<sessionId>/test_visit_basic_auth_secured_page_ChromeTests.mp4

File name will be trimmed to 255 characters to avoid long file names. Moreover, space character will be replaced by _ and only characters alphabets, numbers, - (hyphen), _ (underscore) are retained in the file name.

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

  • Refactored the handling of environment variables in DockerSessionFactory for better clarity and separation of concerns.
  • Introduced functionality to set the video file name based on the se:name capability, enhancing traceability of test artifacts.
  • Implemented sanitization and truncation for video file names to ensure compatibility and prevent excessively long file names.
  • Updated the JDK version in the IDE configuration to JDK 17, aligning with updated language features and support.

Changes walkthrough

Relevant files
Enhancement
DockerSessionFactory.java
Refactor Env Vars Handling and Set Video File Name from Test Name

java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java

  • Refactored environment variable setting into separate methods for
    clarity.
  • Added capability to set video file name based on the se:name
    capability in session request.
  • Video file name sanitization and truncation logic added.
  • +41/-16 
    Configuration changes
    misc.xml
    Update Project JDK Version in IDE Configuration                   

    .idea/misc.xml

    • Updated project JDK version from 11 to 17 in IDE configuration.
    +2/-2     

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

    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    Copy link
    Contributor

    PR Description updated to latest commit (065c28a)

    Copy link
    Contributor

    PR Review

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves changes in the handling of environment variables and capabilities, which are critical for the correct functioning of the Selenium Grid. The logic for setting video file names based on test names introduces new methods and modifies existing ones, requiring careful review to ensure compatibility and correctness.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The method getTestName uses regex to sanitize the test name and limits its length to 251 characters. This might inadvertently cut off important information from the test name, especially if the test name contains significant data towards the end.

    Data Loss: The replacement of spaces with underscores and removal of all characters except alphanumerics, hyphens, and underscores in test names might not be compatible with all systems or might lead to loss of important distinguishing information.

    🔒 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.

    Copy link
    Contributor

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Simplify the method of setting environment variables by removing unnecessary map creation.

    Refactor the method setEnvVarsToContainer to directly accept the environment variables
    from the system instead of creating a new map and then filtering it. This will reduce the
    overhead of creating an unnecessary map and streamline the process of setting environment
    variables.

    java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java [313-318]

    -Map<String, String> seEnvVars = System.getenv();
    -seEnvVars.entrySet().stream()
    -    .filter(
    -        entry ->
    -            entry.getKey().startsWith("SE_") || entry.getKey().equalsIgnoreCase("LANGUAGE"))
    +System.getenv().entrySet().stream()
    +    .filter(entry -> entry.getKey().startsWith("SE_") || entry.getKey().equalsIgnoreCase("LANGUAGE"))
         .forEach(entry -> envVars.put(entry.getKey(), entry.getValue()));
     
    Add specific exception handling for better error management and debugging.

    Consider using a more specific exception handling for the TimeZone and Dimension retrieval
    in the setCapsToEnvVars method. This will help in debugging and maintaining the code by
    providing more detailed error information if the capabilities do not contain the expected
    types or values.

    java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java [330-331]

    -Optional<TimeZone> timeZone = ofNullable(getTimeZone(sessionRequestCapabilities));
    -timeZone.ifPresent(zone -> envVars.put("TZ", zone.getID()));
    +try {
    +    TimeZone timeZone = getTimeZone(sessionRequestCapabilities).orElseThrow(() -> new IllegalArgumentException("Time zone not specified in session capabilities"));
    +    envVars.put("TZ", timeZone.getID());
    +} catch (IllegalArgumentException e) {
    +    // Handle the case where time zone is not provided or invalid
    +    // Log or throw further as needed
    +}
     
    Use regex and utility method for string operations to clean and truncate the test name.

    Instead of manually replacing characters and checking the length in getTestName, consider
    using a regex pattern to clean the string and a utility method to truncate it. This
    approach is cleaner and reduces the complexity within the method.

    java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java [385-389]

    -name = name.replaceAll(" ", "_").replaceAll("[^a-zA-Z0-9_-]", "");
    -if (name.length() > 251) {
    -    name = name.substring(0, 251);
    -}
    +name = name.replaceAll("[^\\w-]", "_");
    +name = StringUtils.abbreviate(name, 251);
     
    Bug
    Add null check before calling toString() to prevent NullPointerException.

    To ensure the robustness of the getTestName method, add a null check for the capability
    retrieval before calling toString() on it. This prevents potential NullPointerException if
    the capability is not set.

    java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java [382-384]

     Optional<Object> testName = ofNullable(sessionRequestCapabilities.getCapability("se:name"));
    -if (testName.isPresent()) {
    +if (testName.isPresent() && testName.get() != null) {
         String name = testName.get().toString();
     
    Maintainability
    Refactor repeated code into a common method to improve maintainability.

    Refactor the repeated code for setting environment variables in getBrowserContainerEnvVars
    and getVideoContainerEnvVars by creating a common method that both can call. This will
    reduce code duplication and improve maintainability.

    java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java [371-374]

    -setEnvVarsToContainer(envVars);
    -setCapsToEnvVars(sessionRequestCapabilities, envVars);
    +setCommonEnvVars(sessionRequestCapabilities, envVars);
     

    ✨ 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
    Copy link
    Member

    diemol commented May 6, 2024

    I cannot update the fork with the latest from trunk.

    @VietND96
    Copy link
    Member Author

    VietND96 commented May 6, 2024

    @diemol, I synced this, can you review this first?

    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, @VietND96!

    .idea/misc.xml Outdated Show resolved Hide resolved
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @diemol diemol merged commit 8e3e26e into SeleniumHQ:trunk May 6, 2024
    35 of 36 checks passed
    @diemol diemol added this to the 4.21 milestone May 6, 2024
    @VietND96 VietND96 deleted the node-docker branch May 6, 2024 12:09
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants