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
Possible Bug The new code adds support for PAC proxy URL, but it's added as a fallback option after SSL and HTTP proxies. This might not be the correct priority order for proxy settings.
Validate the URL before adding it to the arguments list
Consider adding a check to ensure that the proxyAutoconfigUrl is a valid URL before adding it to the arguments list. This can prevent potential issues with invalid PAC proxy URLs.
-} else if (proxy.getProxyAutoconfigUrl() != null) {+} else if (proxy.getProxyAutoconfigUrl() != null && isValidUrl(proxy.getProxyAutoconfigUrl())) {
arguments.add(proxy.getProxyAutoconfigUrl());
+}
Apply this suggestion
Suggestion importance[1-10]: 8
Why: Adding a validation check for the PAC proxy URL ensures that only valid URLs are added to the arguments list, preventing potential runtime errors and improving robustness.
8
Enhancement
Add a specific argument flag for the PAC proxy URL
Consider adding a specific argument flag for the PAC proxy URL, such as "--pac-proxy", to distinguish it from other proxy types. This can improve clarity and make it easier for the Selenium Manager to interpret the arguments correctly.
} else if (proxy.getProxyAutoconfigUrl() != null) {
+ arguments.add("--pac-proxy");
arguments.add(proxy.getProxyAutoconfigUrl());
Apply this suggestion
Suggestion importance[1-10]: 7
Why: Introducing a specific argument flag for the PAC proxy URL enhances clarity and helps distinguish it from other proxy types, which can aid in better argument interpretation by the Selenium Manager.
This file contains hidden or 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
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.
User description
Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
Fixes #14503
Motivation and Context
Adding the PAC proxy URL to arguments for Selenium manager to configure proxy correctly.
Types of changes
Checklist
PR Type
Bug fix
Description
Changes walkthrough 📝
DriverFinder.java
Add PAC proxy URL handling in DriverFinder argumentsjava/src/org/openqa/selenium/remote/service/DriverFinder.java