Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[grid] Improve SlotMatcher and SlotSelector on request browserVersion #14914

Merged
merged 7 commits into from
Dec 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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
Expand Down
8 changes: 8 additions & 0 deletions java/src/org/openqa/selenium/grid/data/NodeStatus.java
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<>();
Expand Down Expand Up @@ -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());
Expand Down
Loading