SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
30.42k stars 8.15k forks source link

[JS]: Handle optional dependency for @bazel/runfiles #14352

Closed harsha509 closed 1 month ago

harsha509 commented 1 month ago

User description

Description

In an npm project, requiring @bazel/runfiles with const { runfiles } = require('@bazel/runfiles') throws an exception because @bazel/runfiles is not available outside of Bazel builds.

The fix safely attempts to require @bazel/runfiles and logs a message if it fails.

Types of changes

Checklist


PR Type

Bug fix


Description


Changes walkthrough 📝

Relevant files
Bug fix
index.js
Handle optional dependency for `@bazel/runfiles` in tests

javascript/node/selenium-webdriver/testing/index.js
  • Removed direct import of @bazel/runfiles.
  • Added a try-catch block to optionally require @bazel/runfiles.
  • Logged error messages if @bazel/runfiles is not available.
  • Set runfiles to null if the module is not available.
  • +12/-1   

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

    codiumai-pr-agent-pro[bot] commented 1 month ago

    PR Reviewer Guide 🔍

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

    Error Handling
    Consider refining the error handling by specifying different actions or messages based on the type of error when requiring `@bazel/runfiles`. This can help in understanding the specific issue and how to resolve it, rather than a generic error message.
    codiumai-pr-agent-pro[bot] commented 1 month ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Consolidate multiple error messages into a single console.error call for clarity ___ **Refactor the error handling for requiring @bazel/runfiles to avoid multiple
    console.error calls and consolidate the error message into a single output. This
    will make the error logs cleaner and more manageable.** [javascript/node/selenium-webdriver/testing/index.js [48-57]](https://github.com/SeleniumHQ/selenium/pull/14352/files#diff-a73b91b68f7208515f860a1e2558954d2c8cfbb516a78bf0e272082da96f079fR48-R57) ```diff try { // Attempt to require @bazel/runfiles runfiles = require('@bazel/runfiles').runfiles; } catch (error) { - // Handle error if @bazel/runfiles is not supported by mocha - console.error('Error requiring @bazel/runfiles:', error.message); - console.error('Note: If you are running tests with Mocha or Jasmine, this module is not needed.'); - console.error('For more details, see: https://github.com/bazelbuild/rules_nodejs/issues/3770'); + const errorMessage = `Error requiring @bazel/runfiles: ${error.message}\n` + + `Note: If you are running tests with Mocha or Jasmine, this module is not needed.\n` + + `For more details, see: https://github.com/bazelbuild/rules_nodejs/issues/3770`; + console.error(errorMessage); runfiles = null; // Set to null if not available } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: The suggestion improves code maintainability by consolidating multiple `console.error` calls into a single call, making the error logs cleaner and easier to manage. This is a minor improvement but beneficial for readability and debugging.
    7
    pujagani commented 1 month ago

    I have faced this few times. I think this looks good to me.