Skip to content

[javascript] Feat 14045/nodejs env var driver location #15930

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

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Jun 21, 2025

User description

🔗 Related Issues

fixes #14045

💥 What does this PR do?

Here I made the same changes as in issue #15653

🔧 Implementation Notes

This pull request introduces support for environment variables to specify the paths of WebDriver executables for various browsers. It also includes corresponding test cases to validate this functionality. The changes ensure that an environment variable can be used to set the executable path, with explicit paths taking precedence, and fallback behavior when the environment variable is not set.

Environment Variable Support for WebDriver Executables:

  • ChromeDriver:

    • Added SE_CHROMEDRIVER environment variable support to specify the ChromeDriver executable path. If not set, the builder falls back to locating the executable on the system PATH. (javascript/selenium-webdriver/chrome.js, [1] [2]
    • Added test cases to verify environment variable behavior for ChromeDriver. (javascript/selenium-webdriver/test/chrome/service_test.js, javascript/selenium-webdriver/test/chrome/service_test.jsR44-R88)
  • EdgeDriver:

    • Added SE_EDGEDRIVER environment variable support to specify the EdgeDriver executable path. Similar fallback behavior as ChromeDriver. (javascript/selenium-webdriver/edge.js, [1] [2]
    • Added test cases to verify environment variable behavior for EdgeDriver. (javascript/selenium-webdriver/test/edge/service_test.js, javascript/selenium-webdriver/test/edge/service_test.jsR42-R86)
  • GeckoDriver (Firefox):

    • Added SE_GECKODRIVER environment variable support for the GeckoDriver executable path. (javascript/selenium-webdriver/firefox.js, [1] [2]
    • Introduced a new test file to validate environment variable behavior for GeckoDriver. (javascript/selenium-webdriver/test/firefox/service_test.js, javascript/selenium-webdriver/test/firefox/service_test.jsR1-R90)
  • IEDriver:

    • Added SE_IEDRIVER environment variable support for the IEDriver executable path. (javascript/selenium-webdriver/ie.js, [1] [2]
    • Introduced a new test file to validate environment variable behavior for IEDriver. (javascript/selenium-webdriver/test/ie/service_test.js, javascript/selenium-webdriver/test/ie/service_test.jsR1-R93)
  • SafariDriver:

    • Added SE_SAFARIDRIVER environment variable support for the SafariDriver executable path. (javascript/selenium-webdriver/safari.js, [1] [2]
    • Added test cases to verify environment variable behavior for SafariDriver. (javascript/selenium-webdriver/test/safari_test.js, javascript/selenium-webdriver/test/safari_test.jsR41-R85)

💡 Additional Considerations

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement, Tests


Description

  • Add environment variable support for WebDriver executables

  • Support SE_CHROMEDRIVER, SE_EDGEDRIVER, SE_GECKODRIVER, SE_IEDRIVER, SE_SAFARIDRIVER

  • Explicit paths override environment variables

  • Comprehensive test coverage for all browsers


Changes walkthrough 📝

Relevant files
Enhancement
5 files
chrome.js
Add SE_CHROMEDRIVER environment variable support                 
+9/-2     
edge.js
Add SE_EDGEDRIVER environment variable support                     
+9/-2     
firefox.js
Add SE_GECKODRIVER environment variable support                   
+9/-2     
ie.js
Add SE_IEDRIVER environment variable support                         
+4/-2     
safari.js
Add SE_SAFARIDRIVER environment variable support                 
+9/-2     
Tests
5 files
service_test.js
Add environment variable tests for ChromeDriver                   
+45/-0   
service_test.js
Add environment variable tests for EdgeDriver                       
+45/-0   
service_test.js
Create new test file for GeckoDriver                                         
+90/-0   
service_test.js
Create new test file for IEDriver                                               
+93/-0   
safari_test.js
Add environment variable tests for SafariDriver                   
+45/-0   

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added the C-nodejs JavaScript Bindings label Jun 21, 2025
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Test Inconsistency

    The IE driver tests use serviceBuilder.getExecutable() instead of service.getExecutable() like other browser tests, which may indicate a different API or potential test error.

      assert.strictEqual(serviceBuilder.getExecutable(), testPath)
    })
    
    it('explicit path overrides environment variable', function () {
      const envPath = '/env/path/to/iedriver'
      const explicitPath = '/explicit/path/to/iedriver'
    
      process.env.SE_IEDRIVER = envPath
      const serviceBuilder = new ie.ServiceBuilder(explicitPath)
    
      assert.strictEqual(serviceBuilder.getExecutable(), explicitPath)
    })
    
    it('falls back to default behavior when environment variable is not set', function () {
      delete process.env.SE_IEDRIVER
    
      const serviceBuilder = new ie.ServiceBuilder()
      // Should be null/undefined when no explicit path and no env var
      assert.ok(!serviceBuilder.getExecutable())

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 21, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Build service before accessing executable

    Call build() method on the service builder before accessing the executable to
    ensure proper initialization. This matches the pattern used in other test files
    and ensures consistent behavior.

    javascript/selenium-webdriver/test/ie/service_test.js [68-69]

     const serviceBuilder = new ie.ServiceBuilder()
    -assert.strictEqual(serviceBuilder.getExecutable(), testPath)
    +const service = serviceBuilder.build()
    +assert.strictEqual(service.getExecutable(), testPath)
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies an inconsistency in the test file javascript/selenium-webdriver/test/ie/service_test.js. All other similar test files in the PR first call .build() on the ServiceBuilder instance and then call .getExecutable() on the resulting service object. This change makes the test consistent with the others and verifies the executable path on the final service object, which is more robust.

    Medium
    • Update

    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.

    Thanks, @iampopovich.

    There is a failing test, can you please check?

    @iampopovich
    Copy link
    Contributor Author

    iampopovich commented Jun 23, 2025

    https://github.com/SeleniumHQ/selenium/actions/runs/15822768191/job/44595655636?pr=15930

    So far, I've discovered two failing tests:

    //javascript/selenium-webdriver:test-chrome-service-test.js-chrome - Error: spawn linux_chromedriver/chromedriver ENOENT

    //javascript/selenium-webdriver:test-builder-test.js-firefox - Error: spawn linux_geckodriver/geckodriver ENOENT

    Both pass locally. I'm trying to determine whether this is a test issue or an environment problem.

    Here's an example of a test run and its console output:

    gitpod /workspace/selenium (feat-14045/nodejs-env-var-driver-location) $ bazel test //javascript/selenium-webdriver:test-chrome-service-test.js-chrome
    ...
    INFO: Found 1 test target...
    Target //javascript/selenium-webdriver:test-chrome-service-test.js-chrome up-to-date:
      bazel-bin/javascript/selenium-webdriver/test-chrome-service-test.js-chrome_/test-chrome-service-test.js-chrome
    INFO: Elapsed time: 59.078s, Critical Path: 2.30s
    INFO: 1808 processes: 1163 disk cache hit, 640 internal, 5 linux-sandbox.
    INFO: Build completed successfully, 1808 total actions
    //javascript/selenium-webdriver:test-chrome-service-test.js-chrome (cached) PASSED in 3.7s
    
    Executed 0 out of 1 test: 1 test passes.
    There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.
    gitpod /workspace/selenium (feat-14045/nodejs-env-var-driver-location) $
    

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [🚀 Feature]: Use Environment Variables to set driver locations
    3 participants