Skip to content

Conversation

@harsha509
Copy link
Member

@harsha509 harsha509 commented Jun 23, 2024

User description

Add step to install SafarTechPreview

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

  • Added a step to install Safari Technology Preview on macOS in the GitHub Actions workflow.
  • Included a check to ensure safaridriver exists and exit if it is not found.

Changes walkthrough 📝

Relevant files
Enhancement
ruby-examples.yml
Add Safari Technology Preview installation and configuration steps

.github/workflows/ruby-examples.yml

  • Added step to install Safari Technology Preview on macOS.
  • Added check for safaridriver existence and exit if not found.
  • +12/-0   

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

    Add step to install SafarTechPreview
    @netlify
    Copy link

    netlify bot commented Jun 23, 2024

    👷 Deploy request for selenium-dev pending review.

    Visit the deploys page to approve it

    Name Link
    🔨 Latest commit d67b12b

    @qodo-merge-pro
    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 PR adds a check for safaridriver in the macOS-latest environment, but it does not handle the scenario where the installation of Safari Technology Preview might fail. Consider adding error handling for the installation step.

    @qodo-merge-pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a step to install Homebrew before installing Safari Technology Preview to ensure the brew command is available

    To ensure the brew command is available, add a step to install Homebrew before attempting
    to install Safari Technology Preview. This prevents potential failures on systems where
    Homebrew is not pre-installed.

    .github/workflows/ruby-examples.yml [34-37]

    +- name: Install Homebrew
    +  if: matrix.os == 'macos-latest'
    +  run: |
    +    /bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)"
     - name: Install Safari Technology Preview
       if: matrix.os == 'macos-latest'
       run: |
         brew install --cask safari-technology-preview
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This is a crucial suggestion as it ensures that the necessary package manager is available before attempting to install Safari Technology Preview, preventing potential failures.

    8
    Use a more specific condition to check if safaridriver is executable

    Use a more specific condition to check for the existence of safaridriver by verifying the
    exact path to the executable.

    .github/workflows/ruby-examples.yml [91-94]

    -if [[ ! -f "/Applications/Safari Technology Preview.app/Contents/MacOS/safaridriver" ]]; then
    -  echo "safaridriver not found. Exiting."
    +if [[ ! -x "/Applications/Safari Technology Preview.app/Contents/MacOS/safaridriver" ]]; then
    +  echo "safaridriver not found or not executable. Exiting."
       exit 1
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: This suggestion enhances the reliability of the check by ensuring that safaridriver is not only present but also executable, which is important for the intended operation.

    6
    Enhancement
    Combine the installation and configuration of Safari Technology Preview into a single step

    Combine the installation and configuration of Safari Technology Preview into a single step
    to streamline the workflow and reduce redundancy.

    .github/workflows/ruby-examples.yml [34-37]

    -- name: Install Safari Technology Preview
    +- name: Install and Configure Safari Technology Preview
       if: matrix.os == 'macos-latest'
       run: |
         brew install --cask safari-technology-preview
    -- name: Install and Configure Safari and WebDriver
    -  if: matrix.os == 'macos-latest'
    -  run: |
         # Check if safaridriver exists
         if [[ ! -f "/Applications/Safari Technology Preview.app/Contents/MacOS/safaridriver" ]]; then
           echo "safaridriver not found. Exiting."
           exit 1
         fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion improves the workflow by reducing redundancy and streamlining the process, which is beneficial for maintainability and clarity.

    7
    Best practice
    Add continue-on-error: true to the step checking for safaridriver to prevent workflow failure

    Add a continue-on-error: true property to the step that checks for safaridriver to prevent
    the entire workflow from failing if Safari Technology Preview is not installed.

    .github/workflows/ruby-examples.yml [87-94]

     - name: Install and Configure Safari and WebDriver
       if: matrix.os == 'macos-latest'
    +  continue-on-error: true
       run: |
         # Check if safaridriver exists
         if [[ ! -f "/Applications/Safari Technology Preview.app/Contents/MacOS/safaridriver" ]]; then
           echo "safaridriver not found. Exiting."
           exit 1
         fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding continue-on-error: true is a good practice to prevent the entire workflow from failing due to a non-critical step, enhancing robustness.

    6

    @harsha509 harsha509 merged commit 34cdad7 into SeleniumHQ:trunk Jun 23, 2024
    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.

    1 participant