You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Possible Bug: The changes in resources.js introduce a new way to handle file paths using runfiles.resolve(filePath). This could potentially change the behavior of the exports.locate function, especially if the runfiles.resolve fails and falls through to the error. It's important to ensure that this new approach does not break existing functionality or introduce path resolution issues in different environments.
Refactoring Needed: The PR includes changes to import paths across multiple test files, changing them from relative to absolute imports using the 'selenium-webdriver' package. This is a significant change that could impact how modules are resolved and loaded. Thorough testing is needed to ensure that these changes do not affect the functionality of the tests and that all modules are correctly located and loaded in all supported environments.
Reduce code duplication by extracting browser setup logic into a separate function
Extract the repeated logic for setting up browser services and options into a separate function to reduce code duplication and improve maintainability.
-if ('SE_CHROMEDRIVER' in process.env) {- const found = locate(process.env.SE_CHROMEDRIVER)- const service = new chrome.ServiceBuilder(found)- builder.setChromeService(service)+function setupBrowser(browserName, envVars, builder) {+ if (envVars.driver in process.env) {+ const found = locate(process.env[envVars.driver])+ const service = new browserName.ServiceBuilder(found)+ builder[`set${browserName}Service`](service)+ }+ if (envVars.binary in process.env) {+ const binary = locate(process.env[envVars.binary])+ const options = new browserName.Options()+ options.setBinary(binary)+ options.setAcceptInsecureCerts(true)+ options.addArguments('disable-infobars', 'disable-breakpad', 'disable-dev-shm-usage', 'no-sandbox')+ builder[`set${browserName}Options`](options)+ }
}
-if ('SE_CHROME' in process.env) {- const binary = locate(process.env.SE_CHROME)- const options = new chrome.Options()- options.setChromeBinaryPath(binary)- options.setAcceptInsecureCerts(true)- options.addArguments('disable-infobars', 'disable-breakpad', 'disable-dev-shm-usage', 'no-sandbox')- builder.setChromeOptions(options)-}+setupBrowser(chrome, { driver: 'SE_CHROMEDRIVER', binary: 'SE_CHROME' }, builder);
Apply this suggestion
Suggestion importance[1-10]: 9
Why: This suggestion significantly improves maintainability by reducing code duplication and encapsulating the browser setup logic into a reusable function. It addresses a major concern in terms of code maintainability.
9
Reduce the number of import statements by using destructuring
Consider destructuring the selenium-webdriver imports to maintain consistency and reduce the number of import statements.
Why: Combining similar imports into a single line improves code readability and maintainability, making it easier to manage.
8
Verify the necessity of the "supports-color" dependency and remove if not needed
Ensure that the newly added dependency "supports-color" is necessary for the project. If it is not explicitly required, consider removing it to keep the package lightweight and to reduce the potential attack surface.
Why: Ensuring that all dependencies are necessary helps keep the package lightweight and reduces the potential attack surface. This suggestion is good for maintainability but requires verification of the dependency's necessity.
7
Best practice
Add a resolution strategy to handle version conflicts and ensure dependency consistency
Consider adding a resolution strategy in the package.json for handling potential version conflicts among dependencies, especially since multiple new dependencies with broad version ranges are being introduced.
Why: Adding a resolution strategy can help manage potential version conflicts among dependencies, ensuring consistency and stability in the project. This is a proactive approach to dependency management.
9
Pin the version of "@bazel/runfiles" to a specific version for more reliable builds
Consider pinning the version of "@bazel/runfiles" to a specific version rather than using a version range. This can help ensure consistent builds and avoid potential issues with automatic updates that might introduce breaking changes.
Why: Pinning the version of "@bazel/runfiles" can help ensure consistent builds and avoid potential issues with automatic updates that might introduce breaking changes. This is a best practice for maintaining stability.
8
Simplify module imports by using consistent and less deep path references
Use consistent import paths for modules from 'selenium-webdriver', avoiding deep path references where possible.
-throw new Error('Unable to locate (no repo mapping file): ' + fileLike)+throw new Error(`Unable to locate (no repo mapping file): ${fileLike}`)
Apply this suggestion
Suggestion importance[1-10]: 6
Why: Using template literals for error messages enhances readability and maintainability. This is a good practice but is a minor improvement in the context of the overall code.
6
Use more descriptive variable names to enhance code clarity
Consider using a more descriptive variable name than found to enhance code clarity.
-const found = locate(process.env.SE_CHROMEDRIVER)-const service = new chrome.ServiceBuilder(found)+const chromeDriverPath = locate(process.env.SE_CHROMEDRIVER)+const service = new chrome.ServiceBuilder(chromeDriverPath)
builder.setChromeService(service)
Apply this suggestion
Suggestion importance[1-10]: 6
Why: More descriptive variable names improve code clarity and readability. This is a minor enhancement but contributes positively to the codebase.
6
Use shallower paths for module imports to enhance manageability and cleanliness of code
Avoid using deep paths for importing modules to make the import statements cleaner and more manageable.
Why: Using shallower paths for imports makes the code cleaner and more manageable, though the improvement is relatively minor.
6
Add visibility attribute to js_library rule
Consider adding a visibility attribute to the js_library rule to restrict or allow access as needed. This helps in managing package accessibility across different parts of the Bazel build environment.
Why: Adding a visibility attribute is a best practice for managing package accessibility in Bazel, but it is not crucial for the functionality of the code.
6
Possible issue
Ensure version consistency in the dependency specifier
Ensure that the version specifier for @bazel/runfiles is consistent with the version number. The specifier uses a caret (^), which allows minor updates, but the version is specified exactly as 5.8.1. If the intention is to allow minor updates, the version should also reflect this flexibility.
Why: The suggestion addresses a potential issue with version consistency, which can prevent unexpected behavior during dependency resolution. This is important for maintaining a stable build environment.
8
Review the necessity of adding [email protected] to multiple dependencies
The addition of [email protected] to various dependencies should be reviewed for necessity and potential version conflicts, especially since it is added globally to many packages. This could lead to dependency resolution issues if different packages require different versions of supports-color.
Why: The suggestion highlights a potential issue with adding the same dependency version across multiple packages, which could lead to conflicts. This is a valid concern for dependency management.
7
Compatibility
Check and pin the version of "sinon" to ensure compatibility with the project's existing dependencies
Review the version constraints for all newly added dependencies to ensure they are compatible with the existing ecosystem of the project, particularly focusing on "sinon" which has a major version update.
Why: Pinning the version of "sinon" can help avoid compatibility issues with the project's existing dependencies, especially since it involves a major version update. This is important for maintaining compatibility.
8
Enhancement
Improve code readability by using destructuring for environment variables
Consider using destructuring for the process.env object to make the code cleaner and more readable.
-if ('SE_CHROMEDRIVER' in process.env) {- const found = locate(process.env.SE_CHROMEDRIVER)+const { SE_CHROMEDRIVER } = process.env;+if (SE_CHROMEDRIVER) {+ const found = locate(SE_CHROMEDRIVER)
const service = new chrome.ServiceBuilder(found)
builder.setChromeService(service)
}
Apply this suggestion
Suggestion importance[1-10]: 7
Why: The suggestion improves code readability and maintainability by using destructuring, which is a cleaner approach. However, the improvement is minor and does not address any critical issues.
7
Possible bug
Verify the definition and accessibility of the BROWSERS dictionary
Ensure that the BROWSERS dictionary is properly defined and accessible in the scope where it's used, as it's critical for the configuration of environment variables and additional data in the mocha_test rules.
+# Ensure BROWSERS is defined and accessible
] + BROWSERS[browser]["data"],
env = {
"SELENIUM_BROWSER": browser,
} | BROWSERS[browser]["env"],
Apply this suggestion
Suggestion importance[1-10]: 5
Why: The suggestion is valid but somewhat redundant, as the usage of BROWSERS in the code implies it is already defined and accessible. It is more of a reminder than a necessary change.
If you `cd` into `javascript/node/selenium-webdriver` a `pnpm`-based workflow should still work.
This PR also removes a now unused ci-javascript.yml file: everything that this used to do is now handled by the RBE.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Type
Enhancement, Tests, Configuration changes
Changes walkthrough 📝
1 files
LocateNodesTest.java
Remove trailing whitespace in `LocateNodesTest.java`
java/test/org/openqa/selenium/bidi/browsingcontext/LocateNodesTest.java
57 files
pinnedScript.js
Add ESLint directive in `pinnedScript.js`
javascript/node/selenium-webdriver/lib/pinnedScript.js
httpserver.js
Update import paths in `httpserver.js`
javascript/node/selenium-webdriver/lib/test/httpserver.js
selenium-webdriver
package.index.js
Update import paths in `index.js`
javascript/node/selenium-webdriver/lib/test/index.js
selenium-webdriver
package.resources.js
Enhance resource location with Bazel runfiles
javascript/node/selenium-webdriver/lib/test/resources.js
@bazel/runfiles
import.locate
function to userunfiles
.actions_test.js
Update import paths in `actions_test.js`
javascript/node/selenium-webdriver/test/actions_test.js
selenium-webdriver
package.add_intercept_parameters_test.js
Update import paths in `add_intercept_parameters_test.js`
javascript/node/selenium-webdriver/test/bidi/add_intercept_parameters_test.js
selenium-webdriver
package.bidi_session_test.js
Update import paths in `bidi_session_test.js`
javascript/node/selenium-webdriver/test/bidi/bidi_session_test.js
selenium-webdriver
package.bidi_test.js
Update import paths in `bidi_test.js`
javascript/node/selenium-webdriver/test/bidi/bidi_test.js
selenium-webdriver
package.browser_test.js
Update import paths in `browser_test.js`
javascript/node/selenium-webdriver/test/bidi/browser_test.js
selenium-webdriver
package.browsingcontext_inspector_test.js
Update import paths in `browsingcontext_inspector_test.js`
javascript/node/selenium-webdriver/test/bidi/browsingcontext_inspector_test.js
selenium-webdriver
package.browsingcontext_test.js
Update import paths in `browsingcontext_test.js`
javascript/node/selenium-webdriver/test/bidi/browsingcontext_test.js
selenium-webdriver
package.input_test.js
Update import paths in `input_test.js`
javascript/node/selenium-webdriver/test/bidi/input_test.js
selenium-webdriver
package.local_value_test.js
Update import paths in `local_value_test.js`
javascript/node/selenium-webdriver/test/bidi/local_value_test.js
selenium-webdriver
package.locate_nodes_test.js
Update import paths in `locate_nodes_test.js`
javascript/node/selenium-webdriver/test/bidi/locate_nodes_test.js
selenium-webdriver
package.log_inspector_test.js
Update import paths in `log_inspector_test.js`
javascript/node/selenium-webdriver/test/bidi/log_inspector_test.js
selenium-webdriver
package.network_commands_test.js
Update import paths in `network_commands_test.js`
javascript/node/selenium-webdriver/test/bidi/network_commands_test.js
selenium-webdriver
package.network_test.js
Update import paths in `network_test.js`
javascript/node/selenium-webdriver/test/bidi/network_test.js
selenium-webdriver
package.script_test.js
Update import paths in `script_test.js`
javascript/node/selenium-webdriver/test/bidi/script_test.js
selenium-webdriver
package.setFiles_command_test.js
Update import paths in `setFiles_command_test.js`
javascript/node/selenium-webdriver/test/bidi/setFiles_command_test.js
selenium-webdriver
package.storage_test.js
Update import paths in `storage_test.js`
javascript/node/selenium-webdriver/test/bidi/storage_test.js
selenium-webdriver
package.builder_test.js
Update import paths in `builder_test.js`
javascript/node/selenium-webdriver/test/builder_test.js
selenium-webdriver
package.devtools_test.js
Update import paths in `devtools_test.js`
javascript/node/selenium-webdriver/test/chrome/devtools_test.js
selenium-webdriver
package.options_test.js
Update import paths in `options_test.js`
javascript/node/selenium-webdriver/test/chrome/options_test.js
selenium-webdriver
package.permission_test.js
Update import paths in `permission_test.js`
javascript/node/selenium-webdriver/test/chrome/permission_test.js
selenium-webdriver
package.service_test.js
Update import paths in `service_test.js`
javascript/node/selenium-webdriver/test/chrome/service_test.js
selenium-webdriver
package.cookie_test.js
Update import paths in `cookie_test.js`
javascript/node/selenium-webdriver/test/cookie_test.js
selenium-webdriver
package.devtools_test.js
Update import paths in `devtools_test.js`
javascript/node/selenium-webdriver/test/devtools_test.js
selenium-webdriver
package.options_test.js
Update import paths in `options_test.js`
javascript/node/selenium-webdriver/test/edge/options_test.js
selenium-webdriver
package.service_test.js
Update import paths in `service_test.js`
javascript/node/selenium-webdriver/test/edge/service_test.js
selenium-webdriver
package.elementAccessibleName_test.js
Update import paths in `elementAccessibleName_test.js`
javascript/node/selenium-webdriver/test/elementAccessibleName_test.js
selenium-webdriver
package.elementAriaRole_test.js
Update import paths in `elementAriaRole_test.js`
javascript/node/selenium-webdriver/test/elementAriaRole_test.js
selenium-webdriver
package.element_finding_test.js
Update import paths in `element_finding_test.js`
javascript/node/selenium-webdriver/test/element_finding_test.js
selenium-webdriver
package.execute_script_test.js
Update import paths in `execute_script_test.js`
javascript/node/selenium-webdriver/test/execute_script_test.js
selenium-webdriver
package.addon_test.js
Update import paths in `addon_test.js`
javascript/node/selenium-webdriver/test/firefox/addon_test.js
selenium-webdriver
package.contextSwitching_test.js
Update import paths in `contextSwitching_test.js`
javascript/node/selenium-webdriver/test/firefox/contextSwitching_test.js
selenium-webdriver
package.full_page_screenshot_test.js
Update import paths in `full_page_screenshot_test.js`
javascript/node/selenium-webdriver/test/firefox/full_page_screenshot_test.js
selenium-webdriver
package.options_test.js
Update import paths in `options_test.js`
javascript/node/selenium-webdriver/test/firefox/options_test.js
selenium-webdriver
package.frame_test.js
Update import paths in `frame_test.js`
javascript/node/selenium-webdriver/test/frame_test.js
selenium-webdriver
package.http_test.js
Update import paths in `http_test.js`
javascript/node/selenium-webdriver/test/http/http_test.js
selenium-webdriver
package.util_test.js
Update import paths in `util_test.js`
javascript/node/selenium-webdriver/test/http/util_test.js
selenium-webdriver
package.options_test.js
Update import paths in `options_test.js`
javascript/node/selenium-webdriver/test/ie/options_test.js
selenium-webdriver
package.api_test.js
Update import paths in `api_test.js`
javascript/node/selenium-webdriver/test/lib/api_test.js
selenium-webdriver
package.capabilities_test.js
Update import paths in `capabilities_test.js`
javascript/node/selenium-webdriver/test/lib/capabilities_test.js
selenium-webdriver
package.form_submit_test.js
Update import paths in `form_submit_test.js`
javascript/node/selenium-webdriver/test/lib/form_submit_test.js
selenium-webdriver
package.webdriver_script_test.js
Update import paths in `webdriver_script_test.js`
javascript/node/selenium-webdriver/test/lib/webdriver_script_test.js
selenium-webdriver
package.page_loading_test.js
Update import paths in `page_loading_test.js`
javascript/node/selenium-webdriver/test/page_loading_test.js
selenium-webdriver
package.print_pdf_test.js
Update import paths in `print_pdf_test.js`
javascript/node/selenium-webdriver/test/print_pdf_test.js
selenium-webdriver
package.proxy_test.js
Update import paths in `proxy_test.js`
javascript/node/selenium-webdriver/test/proxy_test.js
selenium-webdriver
package.rect_test.js
Update import paths in `rect_test.js`
javascript/node/selenium-webdriver/test/rect_test.js
selenium-webdriver
package.remote_test.js
Update import paths in `remote_test.js`
javascript/node/selenium-webdriver/test/remote_test.js
selenium-webdriver
package.safari_test.js
Update import paths in `safari_test.js`
javascript/node/selenium-webdriver/test/safari_test.js
selenium-webdriver
package.select_test.js
Update import paths in `select_test.js`
javascript/node/selenium-webdriver/test/select_test.js
selenium-webdriver
package.stale_element_test.js
Update import paths in `stale_element_test.js`
javascript/node/selenium-webdriver/test/stale_element_test.js
selenium-webdriver
package.upload_test.js
Update import paths in `upload_test.js`
javascript/node/selenium-webdriver/test/upload_test.js
selenium-webdriver
package.virtualAuthenticator_test.js
Update import paths in `virtualAuthenticator_test.js`
javascript/node/selenium-webdriver/test/virtualAuthenticator_test.js
selenium-webdriver
package.webComponent_test.js
Update import paths in `webComponent_test.js`
javascript/node/selenium-webdriver/test/webComponent_test.js
selenium-webdriver
package.window_test.js
Update import paths in `window_test.js`
javascript/node/selenium-webdriver/test/window_test.js
selenium-webdriver
package.