[grid] Improve SlotMatcher and SlotSelector on request browserVersion

Signed-off-by: Viet Nguyen Duc <[email protected]>

java/src/org/openqa/selenium/grid/data/CapabilitiesComparator.java

@@ -0,0 +1,76 @@
+// Licensed to the Software Freedom Conservancy (SFC) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The SFC licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.openqa.selenium.grid.data;
+
+public class CapabilitiesComparator {
+
+  private CapabilitiesComparator() {
+    // Utility methods
+  }
+
+  public static int browserVersionComparator(String v1, String v2) {
+    // Custom semver comparator with empty strings first
+    if (v1.isEmpty() && v2.isEmpty()) return 0;
+    if (v1.isEmpty()) return -1; // Empty string comes first
+    if (v2.isEmpty()) return 1;
+
+    String[] parts1 = v1.split("\\.");
+    String[] parts2 = v2.split("\\.");
+
+    int maxLength = Math.max(parts1.length, parts2.length);
+    for (int i = 0; i < maxLength; i++) {
+      String part1 = i < parts1.length ? parts1[i] : "0";
+      String part2 = i < parts2.length ? parts2[i] : part1;
+
+      boolean isPart1Numeric = isNumber(part1);
+      boolean isPart2Numeric = isNumber(part2);
+
+      if (isPart1Numeric && isPart2Numeric) {
+        // Compare numerically
+        int num1 = Integer.parseInt(part1);
+        int num2 = Integer.parseInt(part2);
+        if (num1 != num2) {
+          return Integer.compare(num2, num1); // Descending order
+        }
+      } else if (!isPart1Numeric && !isPart2Numeric) {
+        // Compare lexicographically, case-insensitive
+        int result = part2.compareToIgnoreCase(part1); // Descending order
+        if (result != 0) {
+          return result;
+        }
+      } else {
+        // Numbers take precedence over strings
+        return isPart1Numeric ? -1 : 1;
+      }
+    }
+
+    return 0; // Versions are equal
+  }
+
+  private static boolean isNumber(String str) {
+    if (str == null || str.isEmpty()) {
+      return false;
+    }
+    for (char c : str.toCharArray()) {
+      if (!Character.isDigit(c)) {
+        return false;
+      }
+    }
+    return true;
+  }
+}

java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java

@@ -17,6 +17,8 @@
 
 package org.openqa.selenium.grid.data;
 
+import static org.openqa.selenium.grid.data.CapabilitiesComparator.browserVersionComparator;
+
 import java.io.Serializable;
 import java.util.Arrays;
 import java.util.List;
@@ -82,7 +84,8 @@ public boolean matches(Capabilities stereotype, Capabilities capabilities) {
         (capabilities.getBrowserVersion() == null
                 || capabilities.getBrowserVersion().isEmpty()
                 || Objects.equals(capabilities.getBrowserVersion(), "stable"))
-            || Objects.equals(stereotype.getBrowserVersion(), capabilities.getBrowserVersion());
+            || browserVersionMatch(
+                stereotype.getBrowserVersion(), capabilities.getBrowserVersion());
     boolean platformNameMatch =
         capabilities.getPlatformName() == null
             || Objects.equals(stereotype.getPlatformName(), capabilities.getPlatformName())
@@ -91,6 +94,10 @@ public boolean matches(Capabilities stereotype, Capabilities capabilities) {
     return browserNameMatch && browserVersionMatch && platformNameMatch;
   }
 
+  private Boolean browserVersionMatch(String stereotype, String capabilities) {
+    return browserVersionComparator(stereotype, capabilities) == 0;
+  }
+
   private Boolean initialMatch(Capabilities stereotype, Capabilities capabilities) {
     return stereotype.getCapabilityNames().stream()
         // Matching of extension capabilities is implementation independent. Skip them

java/src/org/openqa/selenium/grid/data/NodeStatus.java

@@ -204,6 +204,14 @@ public long getLastSessionCreated() {
         .orElse(0);
   }
 
+  public String getBrowserVersion() {
+    return slots.parallelStream()
+        .map(slot -> slot.getStereotype().getBrowserVersion())
+        .filter(Objects::nonNull)
+        .min(CapabilitiesComparator::browserVersionComparator)
+        .orElse("");
+  }
+
   @Override
   public boolean equals(Object o) {
     if (!(o instanceof NodeStatus)) {

java/src/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelector.java

@@ -24,6 +24,7 @@
 import java.util.Set;
 import org.openqa.selenium.Capabilities;
 import org.openqa.selenium.grid.config.Config;
+import org.openqa.selenium.grid.data.CapabilitiesComparator;
 import org.openqa.selenium.grid.data.NodeStatus;
 import org.openqa.selenium.grid.data.Slot;
 import org.openqa.selenium.grid.data.SlotId;
@@ -53,6 +54,12 @@ public Set<SlotId> selectSlot(
                 .thenComparingDouble(NodeStatus::getLoad)
                 // Then last session created (oldest first), so natural ordering again
                 .thenComparingLong(NodeStatus::getLastSessionCreated)
+                // Then sort by stereotype browserVersion (descending order). SemVer comparison with
+                // considering empty value at first.
+                .thenComparing(
+                    Comparator.comparing(
+                        NodeStatus::getBrowserVersion,
+                        CapabilitiesComparator::browserVersionComparator))
                 // And use the node id as a tie-breaker.
                 .thenComparing(NodeStatus::getNodeId))
         .flatMap(

java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java

@@ -18,6 +18,7 @@
 package org.openqa.selenium.grid.data;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.openqa.selenium.grid.data.CapabilitiesComparator.browserVersionComparator;
 
 import org.junit.jupiter.api.Test;
 import org.openqa.selenium.Capabilities;
@@ -29,6 +30,27 @@ class DefaultSlotMatcherTest {
 
   private final DefaultSlotMatcher slotMatcher = new DefaultSlotMatcher();
 
+  @Test
+  void testBrowserVersionMatch() {
+    assertThat(browserVersionComparator("", "")).isEqualTo(0);
+    assertThat(browserVersionComparator("", "130.0")).isEqualTo(-1);
+    assertThat(browserVersionComparator("131.0.6778.85", "131")).isEqualTo(0);
+    assertThat(browserVersionComparator("131.0.6778.85", "131.0")).isEqualTo(0);
+    assertThat(browserVersionComparator("131.0.6778.85", "131.0.6778")).isEqualTo(0);
+    assertThat(browserVersionComparator("131.0.6778.85", "131.0.6778.95")).isEqualTo(1);
+    assertThat(browserVersionComparator("130.0", "130.0")).isEqualTo(0);
+    assertThat(browserVersionComparator("130.0", "130")).isEqualTo(0);
+    assertThat(browserVersionComparator("130.0.1", "130")).isEqualTo(0);
+    assertThat(browserVersionComparator("130.0.1", "130.0.1")).isEqualTo(0);
+    assertThat(browserVersionComparator("133.0a1", "133")).isEqualTo(0);
+    assertThat(browserVersionComparator("dev", "Dev")).isEqualTo(0);
+    assertThat(browserVersionComparator("Beta", "beta")).isEqualTo(0);
+    assertThat(browserVersionComparator("130.0.1", "130.0.2")).isEqualTo(1);
+    assertThat(browserVersionComparator("130.1", "130.0")).isEqualTo(-1);
+    assertThat(browserVersionComparator("131.0", "130.0")).isEqualTo(-1);
+    assertThat(browserVersionComparator("130.0", "131")).isEqualTo(1);
+  }
+
   @Test
   void fullMatch() {
     Capabilities stereotype =

java/test/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelectorTest.java

@@ -87,6 +87,43 @@ void numberOfSupportedBrowsersByNodeIsCorrect() {
     assertThat(supportedBrowsersByNode).isEqualTo(1);
   }
 
+  @Test
+  void nodesAreOrderedNodesByBrowserVersion() {
+    Capabilities caps = new ImmutableCapabilities("browserName", "chrome");
+
+    NodeStatus node1 =
+        createNodeWithStereotypes(
+            Arrays.asList(
+                ImmutableMap.of("browserName", "chrome", "browserVersion", "131.0"),
+                ImmutableMap.of("browserName", "chrome", "browserVersion", "132.0")));
+    NodeStatus node2 =
+        createNodeWithStereotypes(
+            Arrays.asList(ImmutableMap.of("browserName", "chrome", "browserVersion", "131.0")));
+    NodeStatus node3 =
+        createNodeWithStereotypes(
+            Arrays.asList(ImmutableMap.of("browserName", "chrome", "browserVersion", "")));
+    NodeStatus node4 =
+        createNodeWithStereotypes(
+            Arrays.asList(ImmutableMap.of("browserName", "chrome", "browserVersion", "131.1")));
+    NodeStatus node5 =
+        createNodeWithStereotypes(
+            Arrays.asList(ImmutableMap.of("browserName", "chrome", "browserVersion", "beta")));
+    Set<NodeStatus> nodes = ImmutableSet.of(node1, node2, node3, node4, node5);
+
+    Set<SlotId> slots = selector.selectSlot(caps, nodes, new DefaultSlotMatcher());
+
+    ImmutableSet<NodeId> nodeIds =
+        slots.stream().map(SlotId::getOwningNodeId).distinct().collect(toImmutableSet());
+
+    assertThat(nodeIds)
+        .containsSequence(
+            node3.getNodeId(),
+            node1.getNodeId(),
+            node4.getNodeId(),
+            node2.getNodeId(),
+            node5.getNodeId());
+  }
+
   @Test
   void nodesAreOrderedNodesByNumberOfSupportedBrowsers() {
     Set<NodeStatus> nodes = new HashSet<>();
@@ -254,6 +291,20 @@ private NodeStatus createNode(String... browsers) {
     return myNode.getStatus();
   }
 
+  private NodeStatus createNodeWithStereotypes(List<ImmutableMap> stereotypes) {
+    URI uri = createUri();
+    LocalNode.Builder nodeBuilder =
+        LocalNode.builder(tracer, bus, uri, uri, new Secret("cornish yarg"));
+    nodeBuilder.maximumConcurrentSessions(stereotypes.size());
+    stereotypes.forEach(
+        stereotype -> {
+          Capabilities caps = new ImmutableCapabilities(stereotype);
+          nodeBuilder.add(caps, new TestSessionFactory((id, c) -> new Handler(c)));
+        });
+    Node myNode = nodeBuilder.build();
+    return myNode.getStatus();
+  }
+
   private URI createUri() {
     try {
       return new URI("http://localhost:" + random.nextInt());

rust/src/lock.rs

@@ -1,3 +1,20 @@
+// Licensed to the Software Freedom Conservancy (SFC) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The SFC licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
 use crate::logger::Logger;
 use anyhow::Error;
 use std::fs::File;