Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Update java-examples.yml #1919

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

Closed
harsha509 wants to merge 3 commits into SeleniumHQ:trunk from harsha509:trunk
Closed

Update java-examples.yml #1919

harsha509 wants to merge 3 commits into SeleniumHQ:trunk from harsha509:trunk

Conversation

@harsha509
Copy link
Member

@harsha509 harsha509 commented Sep 2, 2024
edited by qodo-merge-pro bot
Loading

User description

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

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

Description

Motivation and Context

Types of changes

  • Change to the site (I have double-checked the Netlify deployment, and my changes look good)
  • Code example added (and I also added the example to all translated languages)
  • Improved translation
  • Added new translation (and I also added a notice to each document missing translation)

Checklist

  • I have read the contributing document.
  • I have used hugo to render the site/docs locally and I am sure it works.

PR Type

enhancement, configuration changes


Description

  • Enhanced the logic for retrieving the latest Selenium snapshot in the Java examples workflow by selecting the snapshot based on the last modified date.
  • Improved the output message to clearly indicate the latest Selenium snapshot version being used.

Changes walkthrough 📝

Relevant files
Enhancement
java-examples.yml
Enhance snapshot retrieval logic in Java examples workflow

.github/workflows/java-examples.yml

  • Updated the method to fetch the latest Selenium snapshot.
  • Added logic to select the snapshot based on the last modified date.
  • Improved output message for clarity.
  • +4/-4

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

    Copy link

    netlify bot commented Sep 2, 2024
    edited
    Loading

    👷 Deploy request for selenium-dev pending review.

    Visit the deploys page to approve it

    Name Link
    🔨 Latest commit 6184804

    Copy link
    Contributor

    qodo-merge-pro bot commented Sep 2, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    Key issues to review

    Potential Error Handling
    The new code doesn't include error handling for the case when the XML content retrieval or parsing fails. This could lead to undefined behavior if the curl command or xq parsing fails.

    Windows Compatibility
    The Windows PowerShell script uses similar logic to the Unix script, but it's not clear if the max_by(.lastModified) function will work the same way in PowerShell. This might lead to inconsistent behavior between Unix and Windows environments.

    Copy link
    Contributor

    qodo-merge-pro bot commented Sep 2, 2024
    edited
    Loading

    PR Code Suggestions ✨

    CategorySuggestion Score
    Error handling
    Add error handling for network requests and data extraction

    Consider adding error handling for the curl command to ensure the script doesn't
    fail silently if the URL is unreachable or returns an unexpected response.

    .github/workflows/java-examples.yml [69-70]

    -xml_content=$(curl -sf https://oss.sonatype.org/service/local/repositories/snapshots/content/org/seleniumhq/selenium/selenium-java/)
    +if ! xml_content=$(curl -sf https://oss.sonatype.org/service/local/repositories/snapshots/content/org/seleniumhq/selenium/selenium-java/); then
    + echo "Failed to fetch XML content"
    + exit 1
    +fi
     latest_snapshot=$(echo $xml_content | xq -r '.content.data."content-item"' | max_by(.lastModified) | .text)
    +if [ -z "$latest_snapshot" ]; then
    + echo "Failed to extract latest snapshot version"
    + exit 1
    +fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion to add error handling for the curl command and data extraction is crucial for ensuring the script does not fail silently, which improves robustness and reliability.

    9
    Validate the retrieved version before using it in subsequent commands

    Consider adding a check to ensure that $latest_snapshot is not empty before using it
    in the Maven command, to prevent potential issues if the snapshot version couldn't
    be retrieved.

    .github/workflows/java-examples.yml [84-87]

     $latest_snapshot = $xml_content.Content | xq -r '.content.data.\"content-item\"' | max_by(.lastModified) | .text
     Write-Output "Latest Selenium Snapshot: $latest_snapshot"
    +if ([string]::IsNullOrEmpty($latest_snapshot)) {
    + Write-Error "Failed to retrieve latest Selenium snapshot version"
    + exit 1
    +}
     cd examples/java
     mvn -B -U test "-Dselenium.version=$latest_snapshot"
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding a check to ensure $latest_snapshot is not empty before using it in the Maven command is important for preventing errors and ensuring the script behaves as expected.

    9
    Performance
    Use built-in PowerShell JSON parsing capabilities instead of external tools

    In the Windows PowerShell section, consider using ConvertFrom-Json instead of xq for
    parsing JSON, as it's a built-in PowerShell cmdlet and doesn't require additional
    installation.

    .github/workflows/java-examples.yml [83-84]

     $xml_content = Invoke-WebRequest -Uri "https://oss.sonatype.org/service/local/repositories/snapshots/content/org/seleniumhq/selenium/selenium-java/"
    -$latest_snapshot = $xml_content.Content | xq -r '.content.data.\"content-item\"' | max_by(.lastModified) | .text
    +$json_content = $xml_content.Content | ConvertFrom-Json
    +$latest_snapshot = ($json_content.content.data.'content-item' | Sort-Object -Property lastModified -Descending | Select-Object -First 1).text
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using ConvertFrom-Json is a good suggestion as it leverages built-in PowerShell capabilities, reducing dependencies and potentially improving performance.

    8

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    1 participant

    AltStyle によって変換されたページ (->オリジナル) /