[grid] Improve SlotMatcher and SlotSelector on request browserVersion (…

…#14914) * [grid] Improve SlotMatcher and SlotSelector on request browserVersion Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com> * Fix tests Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com> --------- Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com> Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
SeleniumHQ · Dec 26, 2024 · dc952cd · dc952cd
1 parent b49678d
commit dc952cd
Show file tree

Hide file tree

Showing 6 changed files with 169 additions and 1 deletion.
diff --git a/java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java b/java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.java
@@ -82,7 +82,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 +92,10 @@ public boolean matches(Capabilities stereotype, Capabilities capabilities) {
     return browserNameMatch && browserVersionMatch && platformNameMatch;
   }
 
+  private boolean browserVersionMatch(String stereotype, String capabilities) {
+    return new SemanticVersionComparator().compare(stereotype, capabilities) == 0;
+  }
+
   private Boolean initialMatch(Capabilities stereotype, Capabilities capabilities) {
     return stereotype.getCapabilityNames().stream()
         // Matching of extension capabilities is implementation independent. Skip them

diff --git a/java/src/org/openqa/selenium/grid/data/NodeStatus.java b/java/src/org/openqa/selenium/grid/data/NodeStatus.java
@@ -204,6 +204,14 @@ public long getLastSessionCreated() {
         .orElse(0);
   }
 
+  public String getBrowserVersion() {
+    return slots.stream()
+        .map(slot -> slot.getStereotype().getBrowserVersion())
+        .filter(Objects::nonNull)
+        .max(new SemanticVersionComparator())
+        .orElse("");
+  }
+
   @Override
   public boolean equals(Object o) {
     if (!(o instanceof NodeStatus)) {

diff --git a/java/src/org/openqa/selenium/grid/data/SemanticVersionComparator.java b/java/src/org/openqa/selenium/grid/data/SemanticVersionComparator.java
@@ -0,0 +1,75 @@
+// 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;
+
+import java.io.Serializable;
+import java.util.Comparator;
+
+public class SemanticVersionComparator implements Comparator<String>, Serializable {
+
+  @Override
+  public int compare(String v1, String v2) {
+    // Custom semver comparator with empty strings last
+    if (v1.isEmpty() && v2.isEmpty()) return 0;
+    if (v1.isEmpty()) return 1; // Empty string comes last
+    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(num1, num2); // Ascending order
+        }
+      } else if (!isPart1Numeric && !isPart2Numeric) {
+        // Compare lexicographically, case-insensitive
+        int result = part1.compareToIgnoreCase(part2); // Ascending order
+        if (result != 0) {
+          return result;
+        }
+      } else {
+        // Numbers take precedence over strings
+        return isPart1Numeric ? 1 : -1;
+      }
+    }
+    return 0; // Versions are equal
+  }
+
+  private boolean isNumber(String str) {
+    if (str == null || str.isEmpty()) {
+      return false;
+    }
+    for (char c : str.toCharArray()) {
+      if (c < '\u0030' || c > '\u0039') {
+        return false;
+      }
+    }
+    return true;
+  }
+}
diff --git a/java/src/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelector.java b/java/src/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelector.java
@@ -26,6 +26,7 @@
 import org.openqa.selenium.Capabilities;
 import org.openqa.selenium.grid.config.Config;
 import org.openqa.selenium.grid.data.NodeStatus;
+import org.openqa.selenium.grid.data.SemanticVersionComparator;
 import org.openqa.selenium.grid.data.Slot;
 import org.openqa.selenium.grid.data.SlotId;
 import org.openqa.selenium.grid.data.SlotMatcher;
@@ -54,6 +55,11 @@ 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, new SemanticVersionComparator().reversed()))
                 // And use the node id as a tie-breaker.
                 .thenComparing(NodeStatus::getNodeId))
         .flatMap(

diff --git a/java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java b/java/test/org/openqa/selenium/grid/data/DefaultSlotMatcherTest.java
@@ -28,6 +28,29 @@
 class DefaultSlotMatcherTest {
 
   private final DefaultSlotMatcher slotMatcher = new DefaultSlotMatcher();
+  private final SemanticVersionComparator comparator = new SemanticVersionComparator();
+
+  @Test
+  void testBrowserVersionMatch() {
+    assertThat(comparator.compare("", "")).isEqualTo(0);
+    assertThat(comparator.compare("", "130.0")).isEqualTo(1);
+    assertThat(comparator.compare("131.0.6778.85", "131")).isEqualTo(0);
+    assertThat(comparator.compare("131.0.6778.85", "131.0")).isEqualTo(0);
+    assertThat(comparator.compare("131.0.6778.85", "131.0.6778")).isEqualTo(0);
+    assertThat(comparator.compare("131.0.6778.85", "131.0.6778.95")).isEqualTo(-1);
+    assertThat(comparator.compare("130.0", "130.0")).isEqualTo(0);
+    assertThat(comparator.compare("130.0", "130")).isEqualTo(0);
+    assertThat(comparator.compare("130.0.1", "130")).isEqualTo(0);
+    assertThat(comparator.compare("130.0.1", "130.0.1")).isEqualTo(0);
+    assertThat(comparator.compare("133.0a1", "133")).isEqualTo(0);
+    assertThat(comparator.compare("133", "133.0a1")).isEqualTo(1);
+    assertThat(comparator.compare("dev", "Dev")).isEqualTo(0);
+    assertThat(comparator.compare("Beta", "beta")).isEqualTo(0);
+    assertThat(comparator.compare("130.0.1", "130.0.2")).isEqualTo(-1);
+    assertThat(comparator.compare("130.1", "130.0")).isEqualTo(1);
+    assertThat(comparator.compare("131.0", "130.0")).isEqualTo(1);
+    assertThat(comparator.compare("130.0", "131")).isEqualTo(-1);
+  }
 
   @Test
   void fullMatch() {

diff --git a/java/test/org/openqa/selenium/grid/distributor/selector/DefaultSlotSelectorTest.java b/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());