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]: video file name set independently in dynamic grid via se:videoName #14148

Merged
merged 2 commits into from
Jun 19, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Jun 18, 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

Motivation and Context

This is an improvement of #13907

In dynamic grid, whenever capability se:name (which shows a test name instead of the session ID in the Grid UI) is set via binding, it is also used to set the output video record file name.

To solve a requirement is

People are setting the cap se:name. However, they don't want this name to be set to a video record file. Or they want an option to control the video record file name independently from the test name shown in the Grid UI.

Capability se:videoName will be used to set video record file name independently. If the capability se:videoName is not set, the se:name is still being used to set for the video record (without regression broken in part of #13907).

For example:

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)')
options.set_capability('se:videoName', 'video')
driver = webdriver.Remote(options=options, command_executor="http://localhost:4444")
driver.get("https://selenium.dev")
driver.quit()

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

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.

PR Type

enhancement


Description

  • Added support for setting video file names independently in the dynamic grid using the se:videoName capability.
  • If se:videoName is not set, the existing se:name capability will be used as a fallback to name the video file.
  • Refactored the method to retrieve capability names for video file naming to improve code maintainability.

Changes walkthrough 📝

Relevant files
Enhancement
DockerSessionFactory.java
Support independent video file naming via `se:videoName` capability.

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

  • Added logic to set video file name independently using se:videoName
    capability.
  • Fallback to se:name capability if se:videoName is not provided.
  • Refactored method to generalize capability name retrieval for video
    file naming.
  • +12/-4   

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 2
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The method getVideoFileName is used to fetch both se:videoName and se:name capabilities. However, the method name suggests it is specifically for getting video file names. Consider renaming the method to reflect its generalized use, such as getCapabilityAsString.
    Code Duplication:
    The logic for sanitizing the name string is duplicated in the getVideoFileName method. Consider extracting this logic into a separate method to improve code maintainability and reduce duplication.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Combine the Optional checks for videoName and testName into a single statement

    Combine the two Optional checks for videoName and testName into a single statement to
    simplify the code and reduce redundancy.

    java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java [376-383]

    -Optional<String> videoName =
    -    ofNullable(getVideoFileName(sessionRequestCapabilities, "se:videoName"));
    -if (videoName.isPresent()) {
    -  envVars.put("SE_VIDEO_FILE_NAME", String.format("%s.mp4", videoName.get()));
    -} else {
    -  Optional<String> testName =
    -      ofNullable(getVideoFileName(sessionRequestCapabilities, "se:name"));
    -  testName.ifPresent(name -> envVars.put("SE_VIDEO_FILE_NAME", String.format("%s.mp4", name)));
    -}
    +Optional<String> videoName = ofNullable(getVideoFileName(sessionRequestCapabilities, "se:videoName"))
    +    .or(() -> ofNullable(getVideoFileName(sessionRequestCapabilities, "se:name")));
    +videoName.ifPresent(name -> envVars.put("SE_VIDEO_FILE_NAME", String.format("%s.mp4", name)));
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion correctly identifies a way to simplify the code by combining two Optional checks into one, reducing redundancy and improving readability.

    8
    Possible bug
    Add a null check for sessionRequestCapabilities before accessing its capabilities

    Add a null check for sessionRequestCapabilities before calling getCapability to prevent
    potential NullPointerException.

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

    +if (sessionRequestCapabilities == null) {
    +  return null;
    +}
     Optional<Object> testName =
         ofNullable(sessionRequestCapabilities.getCapability(capabilityName));
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a null check is a good practice to prevent NullPointerException, especially when dealing with external inputs or configurations. This suggestion enhances the robustness of the code.

    7
    Maintainability
    Use a constant for the video file extension to improve maintainability

    Use a constant for the video file extension to avoid hardcoding the ".mp4" string multiple
    times, which improves maintainability.

    java/src/org/openqa/selenium/grid/node/docker/DockerSessionFactory.java [379-383]

    -envVars.put("SE_VIDEO_FILE_NAME", String.format("%s.mp4", videoName.get()));
    -testName.ifPresent(name -> envVars.put("SE_VIDEO_FILE_NAME", String.format("%s.mp4", name)));
    +final String VIDEO_FILE_EXTENSION = ".mp4";
    +envVars.put("SE_VIDEO_FILE_NAME", String.format("%s" + VIDEO_FILE_EXTENSION, videoName.get()));
    +testName.ifPresent(name -> envVars.put("SE_VIDEO_FILE_NAME", String.format("%s" + VIDEO_FILE_EXTENSION, name)));
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a constant for repeated string literals like file extensions can improve maintainability and make future changes easier. This suggestion is valid and applicable to the new code in the PR.

    6

    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.

    3 participants