[grid] flatten combined routes to improve routing

[joerg1985]

User description

Motivation and Context

Before this PR there are 3 - 4 levels of nested CombinedRoute instances, this PR will flatten them.

A CombinedRoute must call match again inside handle to determinate the correct Route to call. Each match call will create a copy of the HttpRequest to check this, this overhead is reduced by this PR.

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• [x] I have read the contributing document.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.
• [x] I have added tests to cover my changes.
• [ ] All new and existing tests passed.

---

Type

enhancement

---

Description

• Refactored CombinedRoute to flatten nested instances, improving the routing process by reducing the overhead of matching routes.
• Updated tests to align with the new logic of flattened CombinedRoute instances.

---

Changes walkthrough

	Relevant files
Enhancement	Route.java Refactor CombinedRoute to Flatten Nested Instances              java/src/org/openqa/selenium/remote/http/Route.java Added import for ArrayList. Changed rawPrefixes initialization from LinkedList to Collections.emptyList(). Refactored CombinedRoute constructor to flatten nested CombinedRoute instances and reverse their order. +16/-2
Tests	RouteTest.java Update Tests to Reflect Flattened Route Logic                        java/test/org/openqa/selenium/remote/http/RouteTest.java Modified a test to use nested Route.combine to reflect changes in flattening logic. +5/-2

---

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

[qodo-merge-pro[bot]]

PR Description updated to latest commit (https://github.com/SeleniumHQ/selenium/commit/bf2b1659b6e9c054fb6ba44484ecb738a76e40b1)

• [ ] Copy walkthrough table to "Files Changed" Tab

[qodo-merge-pro[bot]]

PR Review

⏱️ Estimated effort to review [1-5]	3, because the changes involve refactoring critical routing logic which requires careful review to ensure that the new flattened structure of routes does not introduce any regressions or change the expected behavior of the routing system.
🧪 Relevant tests	Yes
🔍 Possible issues	Possible Bug: The refactoring changes how CombinedRoute instances are flattened and reversed. This could potentially alter the order in which routes are evaluated, affecting the final routing behavior. It's crucial to ensure that this new ordering still respects the intended overrides and matches.
🔒 Security concerns	No

---

✨ Review tool usage guide:

---

Overview: The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

• When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

• With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

[qodo-merge-pro[bot]]

PR Code Suggestions

Category	Suggestions
Bug	Replace immutable list creation with a modifiable list to prevent runtime exceptions. Replace Collections.emptyList() with new ArrayList<>() to allow modification of the list later if needed. Using Collections.emptyList() returns an immutable list, which will cause an UnsupportedOperationException if any attempt is made to modify it. java/src/org/openqa/selenium/remote/http/Route.java [296] -rawPrefixes = Collections.emptyList(); +rawPrefixes = new ArrayList<>();
Enhancement	Use a specific collection type when collecting streams to ensure mutability. Consider using Collectors.toCollection(ArrayList::new) instead of Collectors.toList() in the stream collection to explicitly control the list implementation. This ensures that the list is mutable and can be modified later if necessary. java/src/org/openqa/selenium/remote/http/Route.java [338] -.collect(Collectors.toList()); +.collect(Collectors.toCollection(ArrayList::new));
Performance	Simplify stream handling by reducing unnecessary list creation. Avoid unnecessary creation of ArrayList inside the lambda expression when handling instances of CombinedRoute. Instead, directly return the stream of allRoutes to reduce overhead and simplify the code. java/src/org/openqa/selenium/remote/http/Route.java [330-334] if (route instanceof CombinedRoute combinedRoute) { - List<Routable> nestedRoutes = new ArrayList<>(combinedRoute.allRoutes); - Collections.reverse(nestedRoutes); - return nestedRoutes.stream(); + return new ArrayList<>(Collections.reverse(combinedRoute.allRoutes)).stream(); }
	Optimize list creation and reversal with a more efficient approach. Use List.of() instead of Collections.reverse() followed by List.copyOf() to create an immutable reversed list in a more concise and efficient manner. java/src/org/openqa/selenium/remote/http/Route.java [339-340] -Collections.reverse(routables); -allRoutes = List.copyOf(routables); +allRoutes = List.of(routables.toArray(new Routable[0]));
Maintainability	Streamline the test case to focus on the essential functionality. Simplify the test case by removing redundant route definitions that do not contribute to the test's purpose, focusing on the overriding behavior of routes. java/test/org/openqa/selenium/remote/http/RouteTest.java [154-159] Route.combine( Route.get("/hello").to(() -> req -> new HttpResponse().setContent(utf8String("world"))), - Route.combine( - Route.get("/hello") - .to(() -> req -> new HttpResponse().setContent(utf8String("world"))), - Route.get("/hello") - .to(() -> req -> new HttpResponse().setContent(utf8String("buddy"))))); + Route.get("/hello").to(() -> req -> new HttpResponse().setContent(utf8String("buddy"))));

---

✨ Improve tool usage guide:

---

Overview: The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

• When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

• With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

[qodo-merge-pro[bot]]

CI Failure Feedback

(Checks updated until commit https://github.com/SeleniumHQ/selenium/commit/11a809308725c35c5d79df16d7a52cded06b9ade)

Action: Ruby / Local Tests (safari, macos) / Local Tests (safari, macos)

Failed stage: Run Bazel [❌]

Failed test name: Selenium::WebDriver::Window can minimize the window

Failure summary: The action failed due to a failing test in the Selenium WebDriver integration suite for Safari. Specifically, the test Selenium::WebDriver::Window can minimize the window failed because it was expected to fail (marked as pending with no specified reason), but no error was raised, indicating a discrepancy between expected and actual behavior.

Relevant error logs: 1: ##[group]Operating System 2: macOS ... 880: /Applications/Xcode_15.0.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: warning renaming duplicate member name 'message.o' from 'bazel-out/darwin_x86_64-opt-exec-ST-13d3ddad9198/bin/external/protobuf~/_objs/protoc_lib/1/message.o(message.o)' and 'bazel-out/darwin_x86_64-opt-exec-ST-13d3ddad9198/bin/external/protobuf~/_objs/protoc_lib/0/message.o(message.o)' 881: /Applications/Xcode_15.0.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: warning renaming duplicate member name 'message_field.o' from 'bazel-out/darwin_x86_64-opt-exec-ST-13d3ddad9198/bin/external/protobuf~/_objs/protoc_lib/0/message_field.o(message_field.o)' and 'bazel-out/darwin_x86_64-opt-exec-ST-13d3ddad9198/bin/external/protobuf~/_objs/protoc_lib/1/message_field.o(message_field.o)' 882: /Applications/Xcode_15.0.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: warning renaming duplicate member name 'primitive_field.o' from 'bazel-out/darwin_x86_64-opt-exec-ST-13d3ddad9198/bin/external/protobuf~/_objs/protoc_lib/1/primitive_field.o(primitive_field.o)' and 'bazel-out/darwin_x86_64-opt-exec-ST-13d3ddad9198/bin/external/protobuf~/_objs/protoc_lib/0/primitive_field.o(primitive_field.o)' 883: /Applications/Xcode_15.0.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: warning renaming duplicate member name 'service.o' from 'bazel-out/darwin_x86_64-opt-exec-ST-13d3ddad9198/bin/external/protobuf~/_objs/protoc_lib/0/service.o(service.o)' and 'bazel-out/darwin_x86_64-opt-exec-ST-13d3ddad9198/bin/external/protobuf~/_objs/protoc_lib/1/service.o(service.o)' 884: /Applications/Xcode_15.0.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: warning renaming duplicate member name 'string_field.o' from 'bazel-out/darwin_x86_64-opt-exec-ST-13d3ddad9198/bin/external/protobuf~/_objs/protoc_lib/0/string_field.o(string_field.o)' and 'bazel-out/darwin_x86_64-opt-exec-ST-13d3ddad9198/bin/external/protobuf~/_objs/protoc_lib/1/string_field.o(string_field.o)' 885: [32m[1,079 / 1,863][0m Compiling src/google/protobuf/descriptor.cc [for tool]; 6s disk-cache, darwin-sandbox 886: [32m[1,080 / 1,863][0m [Prepa] Linking external/protobuf~/libprotobuf.a [for tool] 887: [32mINFO: [0mFrom Linking external/protobuf~/libprotobuf.a [for tool]: 888: /Applications/Xcode_15.0.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool: file: bazel-out/darwin_x86_64-opt-exec-ST-13d3ddad9198/bin/external/protobuf~/_objs/protobuf/error_listener.o has no symbols 889: [32mINFO: [0mFrom Linking external/protobuf~/protoc [for tool]: 890: ld: warning: ignoring duplicate libraries: '-lm', '-lpthread' 891: [32m[1,292 / 1,863][0m Checking 1 JS files in @@io_bazel_rules_closure//closure/library/module:moduleloadcallback; 0s disk-cache ... (4 actions, 0 running) 892: [32m[1,863 / 1,864][0m [Prepa] Testing //rb/spec/integration/selenium/webdriver:error-safari 893: [32m[1,863 / 1,864][0m Testing //rb/spec/integration/selenium/webdriver:error-safari; 1s disk-cache 894: [32m[1,863 / 1,864][0m Testing //rb/spec/integration/selenium/webdriver:error-safari; 0s local, disk-cache 895: [32m[1,863 / 1,864][0m Testing //rb/spec/integration/selenium/webdriver:error-safari; 8s local, disk-cache ... 976: [31m[1mFAIL: [0m//rb/spec/integration/selenium/webdriver:window-safari (see /Users/runner/.bazel/execroot/_main/bazel-out/darwin_x86_64-fastbuild/testlogs/rb/spec/integration/selenium/webdriver/window-safari/test_attempts/attempt_1.log) 977: [32m[1,886 / 1,887][0m 23 / 24 tests;[0m Testing //rb/spec/integration/selenium/webdriver:window-safari; 20s local, disk-cache 978: [32m[1,886 / 1,887][0m 23 / 24 tests;[0m Testing //rb/spec/integration/selenium/webdriver:window-safari; 40s local, disk-cache 979: [31m[1mFAIL: [0m//rb/spec/integration/selenium/webdriver:window-safari (see /Users/runner/.bazel/execroot/_main/bazel-out/darwin_x86_64-fastbuild/testlogs/rb/spec/integration/selenium/webdriver/window-safari/test_attempts/attempt_2.log) 980: [32m[1,886 / 1,887][0m 23 / 24 tests;[0m Testing //rb/spec/integration/selenium/webdriver:window-safari; 41s local, disk-cache 981: [32m[1,886 / 1,887][0m 23 / 24 tests;[0m Testing //rb/spec/integration/selenium/webdriver:window-safari; 61s local, disk-cache 982: [31m[1mFAIL: [0m//rb/spec/integration/selenium/webdriver:window-safari (see /Users/runner/.bazel/execroot/_main/bazel-out/darwin_x86_64-fastbuild/testlogs/rb/spec/integration/selenium/webdriver/window-safari/test.log) 983: ==================== Test output for //rb/spec/integration/selenium/webdriver:window-safari: 984: [31m[1mFAILED: [0m//rb/spec/integration/selenium/webdriver:window-safari (Summary) ... 989: driver: safari 990: version: 17.4.1 991: platform: macosx 992: ci: github 993: ruby: ruby 3.0.6p216 (2023-03-30 revision 23a532679b) [x86_64-darwin21] 994: ........F 995: Failures: 996: 1) Selenium::WebDriver::Window can minimize the window FIXED 997: Expected pending 'Test guarded; no reason given' to fail. No error was raised. 998: # ./rb/spec/integration/selenium/webdriver/window_spec.rb:127 999: Finished in 17.92 seconds (files took 0.26209 seconds to load) 1000: 9 examples, 1 failure 1001: Failed examples: ... 1013: ruby: ruby 3.0.6p216 (2023-03-30 revision 23a532679b) [x86_64-darwin21] 1014: ........F 1015: Failures: 1016: /Users/runner/.bazel/execroot/_main/bazel-out/darwin_x86_64-fastbuild/testlogs/rb/spec/integration/selenium/webdriver/window-safari/test.log 1017: /Users/runner/.bazel/execroot/_main/bazel-out/darwin_x86_64-fastbuild/testlogs/rb/spec/integration/selenium/webdriver/window-safari/test_attempts/attempt_1.log 1018: /Users/runner/.bazel/execroot/_main/bazel-out/darwin_x86_64-fastbuild/testlogs/rb/spec/integration/selenium/webdriver/window-safari/test_attempts/attempt_2.log 1019: [32mINFO: [0mFrom Testing //rb/spec/integration/selenium/webdriver:window-safari: 1020: 1) Selenium::WebDriver::Window can minimize the window FIXED 1021: Expected pending 'Test guarded; no reason given' to fail. No error was raised. 1022: # ./rb/spec/integration/selenium/webdriver/window_spec.rb:127 1023: Finished in 20.23 seconds (files took 0.25834 seconds to load) 1024: 9 examples, 1 failure 1025: Failed examples: ... 1033: driver: safari 1034: version: 17.4.1 1035: platform: macosx 1036: ci: github 1037: ruby: ruby 3.0.6p216 (2023-03-30 revision 23a532679b) [x86_64-darwin21] 1038: ........F 1039: Failures: 1040: 1) Selenium::WebDriver::Window can minimize the window FIXED 1041: Expected pending 'Test guarded; no reason given' to fail. No error was raised. 1042: # ./rb/spec/integration/selenium/webdriver/window_spec.rb:127 1043: Finished in 20.5 seconds (files took 0.26144 seconds to load) 1044: 9 examples, 1 failure 1045: Failed examples: 1046: rspec ./rb/spec/integration/selenium/webdriver/window_spec.rb:127 # Selenium::WebDriver::Window can minimize the window 1047: ================================================================================ 1048: [32mINFO: [0mFound 24 test targets... 1049: [32mINFO: [0mElapsed time: 967.287s, Critical Path: 249.34s 1050: [32mINFO: [0m1887 processes: 799 disk cache hit, 825 internal, 212 darwin-sandbox, 51 local. 1051: [32mINFO: [0mBuild completed, 1 test FAILED, 1887 total actions 1052: //rb/spec/integration/selenium/webdriver:action_builder-safari [0m[32mPASSED[0m in 66.0s 1053: //rb/spec/integration/selenium/webdriver:bidi-safari [0m[32mPASSED[0m in 18.4s 1054: //rb/spec/integration/selenium/webdriver:devtools-safari [0m[32mPASSED[0m in 225.9s 1055: //rb/spec/integration/selenium/webdriver:driver-safari [0m[32mPASSED[0m in 29.9s 1056: //rb/spec/integration/selenium/webdriver:element-safari [0m[32mPASSED[0m in 38.1s 1057: //rb/spec/integration/selenium/webdriver:error-safari [0m[32mPASSED[0m in 8.2s ... 1067: //rb/spec/integration/selenium/webdriver:timeout-safari [0m[32mPASSED[0m in 7.8s 1068: //rb/spec/integration/selenium/webdriver:virtual_authenticator-safari [0m[32mPASSED[0m in 2.2s 1069: //rb/spec/integration/selenium/webdriver:zipper-safari [0m[32mPASSED[0m in 2.4s 1070: //rb/spec/integration/selenium/webdriver/bidi:browsing_context-safari [0m[32mPASSED[0m in 34.7s 1071: //rb/spec/integration/selenium/webdriver/bidi:log_inspector-safari [0m[32mPASSED[0m in 32.7s 1072: //rb/spec/integration/selenium/webdriver/remote:driver-safari [0m[32mPASSED[0m in 7.3s 1073: //rb/spec/integration/selenium/webdriver/remote:element-safari [0m[32mPASSED[0m in 28.3s 1074: //rb/spec/integration/selenium/webdriver/safari:driver-safari [0m[32mPASSED[0m in 6.5s 1075: //rb/spec/integration/selenium/webdriver:window-safari [0m[31m[1mFAILED[0m in 3 out of 3 in 21.4s 1076: Stats over 3 runs: max = 21.4s, min = 18.8s, avg = 20.4s, dev = 1.2s 1077: /Users/runner/.bazel/execroot/_main/bazel-out/darwin_x86_64-fastbuild/testlogs/rb/spec/integration/selenium/webdriver/window-safari/test.log 1078: /Users/runner/.bazel/execroot/_main/bazel-out/darwin_x86_64-fastbuild/testlogs/rb/spec/integration/selenium/webdriver/window-safari/test_attempts/attempt_1.log 1079: /Users/runner/.bazel/execroot/_main/bazel-out/darwin_x86_64-fastbuild/testlogs/rb/spec/integration/selenium/webdriver/window-safari/test_attempts/attempt_2.log 1080: Executed 24 out of 24 tests: 23 tests pass and [0m[31m[1m1 fails locally[0m. 1081: There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are. 1082: [0m 1083: ##[error]Process completed with exit code 3.

---

✨ CI feedback usage guide:

---

The CI feedback tool (/checks) automatically triggers when a PR has a failed check. The tool analyzes the failed checks and provides several feedbacks:

• Failed stage
• Failed test name
• Failure summary
• Relevant error logs

In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

/checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"

where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

Configuration options

• enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
• excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
• enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
• persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
• final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

See more information about the checks tool in the docs.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 58.72%. Comparing base (ec54309) to head (11a8093). Report is 54 commits behind head on trunk.

:exclamation: Current head 11a8093 differs from pull request most recent head f9c040d. Consider uploading reports for the commit f9c040d to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##            trunk   #13856   +/-   ##
=======================================
  Coverage   58.72%   58.72%           
=======================================
  Files          86       86           
  Lines        5298     5298           
  Branches      226      226           
=======================================
  Hits         3111     3111           
  Misses       1961     1961           
  Partials      226      226           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.