Skip to content

Commit 9249e31

Browse files
diemoldiemol
committed
[grid] Reverting fix for #10391
The spec https://www.w3.org/TR/webdriver1/#status does not specify which http code should be returned, but it implies that this endpoint should be available to query the Grid status. Besides, returning 503 will break checks already implemented by users.
1 parent 26978eb commit 9249e31

2 files changed

Lines changed: 79 additions & 140 deletions

File tree

‎java/src/org/openqa/selenium/grid/router/GridStatusHandler.java‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@
4040
import java.util.concurrent.Executors;
4141
import java.util.concurrent.TimeoutException;
4242

43-
import static java.net.HttpURLConnection.HTTP_UNAVAILABLE;
4443
import static java.util.concurrent.TimeUnit.SECONDS;
4544
import static java.util.stream.Collectors.toList;
4645
import static org.openqa.selenium.grid.data.Availability.UP;
@@ -146,15 +145,10 @@ public HttpResponse execute(HttpRequest req) {
146145

147146
HttpResponse res = new HttpResponse()
148147
.setContent(asJson(ImmutableMap.of("value", value.build())));
149-
if (ready) {
150-
span.setStatus(Status.OK);
151-
} else {
152-
res.setStatus(HTTP_UNAVAILABLE);
153-
span.setStatus(Status.UNAVAILABLE);
154-
}
155148
HTTP_RESPONSE.accept(span, res);
156149
HTTP_RESPONSE_EVENT.accept(attributeMap, res);
157150
attributeMap.put("grid.status", EventAttribute.setValue(ready));
151+
span.setStatus(Status.OK);
158152
span.addEvent("Computed grid status", attributeMap);
159153
return res;
160154
}

‎java/test/org/openqa/selenium/grid/router/RouterTest.java‎

Lines changed: 78 additions & 133 deletions
Original file line numberDiff line numberDiff line change
@@ -17,20 +17,6 @@
1717

1818
package org.openqa.selenium.grid.router;
1919

20-
import static org.assertj.core.api.Assertions.assertThat;
21-
import static org.assertj.core.api.Assertions.fail;
22-
import static org.junit.Assert.assertEquals;
23-
import static org.junit.Assert.assertFalse;
24-
import static org.junit.Assert.assertNotNull;
25-
import static org.junit.Assert.assertTrue;
26-
import static org.openqa.selenium.grid.data.Availability.DOWN;
27-
import static org.openqa.selenium.grid.data.Availability.UP;
28-
import static org.openqa.selenium.json.Json.MAP_TYPE;
29-
import static org.openqa.selenium.remote.http.Contents.reader;
30-
import static org.openqa.selenium.remote.Dialect.OSS;
31-
import static org.openqa.selenium.remote.Dialect.W3C;
32-
import static org.openqa.selenium.remote.http.HttpMethod.GET;
33-
3420
import com.google.common.collect.ImmutableMap;
3521
import com.google.common.collect.ImmutableSet;
3622

@@ -62,8 +48,6 @@
6248
import org.openqa.selenium.grid.testing.TestSessionFactory;
6349
import org.openqa.selenium.grid.web.CombinedHandler;
6450
import org.openqa.selenium.grid.web.Values;
65-
import org.openqa.selenium.json.Json;
66-
import org.openqa.selenium.json.JsonInput;
6751
import org.openqa.selenium.internal.Either;
6852
import org.openqa.selenium.remote.http.HttpClient;
6953
import org.openqa.selenium.remote.http.HttpRequest;
@@ -72,8 +56,6 @@
7256
import org.openqa.selenium.remote.tracing.Tracer;
7357
import org.openqa.selenium.support.ui.FluentWait;
7458

75-
import java.io.IOException;
76-
import java.io.Reader;
7759
import java.net.URI;
7860
import java.net.URISyntaxException;
7961
import java.time.Duration;
@@ -83,54 +65,41 @@
8365
import java.util.UUID;
8466
import java.util.concurrent.atomic.AtomicReference;
8567

68+
import static org.assertj.core.api.Assertions.assertThat;
69+
import static org.junit.Assert.assertEquals;
70+
import static org.junit.Assert.assertFalse;
71+
import static org.junit.Assert.assertNotEquals;
72+
import static org.junit.Assert.assertNotNull;
73+
import static org.junit.Assert.assertTrue;
74+
import static org.openqa.selenium.grid.data.Availability.DOWN;
75+
import static org.openqa.selenium.grid.data.Availability.UP;
76+
import static org.openqa.selenium.json.Json.MAP_TYPE;
77+
import static org.openqa.selenium.remote.Dialect.OSS;
78+
import static org.openqa.selenium.remote.Dialect.W3C;
79+
import static org.openqa.selenium.remote.http.HttpMethod.GET;
80+
8681
public class RouterTest {
8782

8883
private Tracer tracer;
8984
private EventBus bus;
90-
private CombinedHandler handler;
91-
private SessionMap sessions;
92-
private NewSessionQueue queue;
9385
private Distributor distributor;
9486
private Router router;
9587
private Secret registrationSecret;
96-
private HttpClient.Factory clientFactory;
97-
private Json JSON = new Json();
98-
99-
private static void waitUntilReady(Router router, Duration duration) {
100-
new FluentWait<>(router)
101-
.withTimeout(duration)
102-
.pollingEvery(Duration.ofMillis(100))
103-
.until(r -> {
104-
HttpResponse response = r.execute(new HttpRequest(GET, "/status"));
105-
Map<String, Object> status = Values.get(response, MAP_TYPE);
106-
return Boolean.TRUE.equals(status.get("ready"));
107-
});
108-
}
109-
110-
private static void waitUntilNotReady(Router router, Duration duration) {
111-
new FluentWait<>(router)
112-
.withTimeout(duration)
113-
.pollingEvery(Duration.ofMillis(100))
114-
.until(r -> {
115-
HttpResponse response = r.execute(new HttpRequest(GET, "/status"));
116-
return response.getStatus()!=200;
117-
});
118-
}
11988

12089
@Before
12190
public void setUp() {
12291
tracer = DefaultTestTracer.createTracer();
12392
bus = new GuavaEventBus();
12493

125-
handler = new CombinedHandler();
126-
clientFactory = new PassthroughHttpClient.Factory(handler);
94+
CombinedHandler handler = new CombinedHandler();
95+
HttpClient.Factory clientFactory = new PassthroughHttpClient.Factory(handler);
12796

128-
sessions = new LocalSessionMap(tracer, bus);
97+
SessionMap sessions = new LocalSessionMap(tracer, bus);
12998
handler.addHandler(sessions);
13099

131100
registrationSecret = new Secret("stinking bishop");
132101

133-
queue = new LocalNewSessionQueue(
102+
NewSessionQueue queue = new LocalNewSessionQueue(
134103
tracer,
135104
bus,
136105
new DefaultSlotMatcher(),
@@ -147,7 +116,7 @@ public void setUp() {
147116
queue,
148117
new DefaultSlotSelector(),
149118
registrationSecret,
150-
Duration.ofMinutes(5),
119+
Duration.ofSeconds(1),
151120
false,
152121
Duration.ofSeconds(5));
153122
handler.addHandler(distributor);
@@ -156,121 +125,89 @@ public void setUp() {
156125
}
157126

158127
@Test
159-
public void shouldListAnEmptyDistributorAsMeaningTheGridIsNotReady() throws IOException {
160-
HttpResponse response = getStatusHttpResponse(router);
161-
assertNotNull(response);
128+
public void shouldListAnEmptyDistributorAsMeaningTheGridIsNotReady() {
162129
Map<String, Object> status = getStatus(router);
163130
assertFalse((Boolean) status.get("ready"));
164131
}
165132

166133
@Test
167-
public void shouldReturnServerErrorCodeWhenGridIsNotReady() {
168-
HttpResponse response = getStatusHttpResponse(router);
169-
assertNotNull(response);
170-
assertEquals(503, response.getStatus());
171-
}
172-
173-
@Test
174-
public void addingANodeThatIsDownMeansTheGridIsNotReady()
175-
throws URISyntaxException, IOException {
176-
distributor = new LocalDistributor(
177-
tracer,
178-
bus,
179-
clientFactory,
180-
sessions,
181-
queue,
182-
new DefaultSlotSelector(),
183-
registrationSecret,
184-
Duration.ofSeconds(1),
185-
false,
186-
Duration.ofSeconds(5));
187-
handler.addHandler(distributor);
188-
189-
router = new Router(tracer, clientFactory, sessions, queue, distributor);
190-
191-
Capabilities capabilities = new ImmutableCapabilities("cheese", "peas");
192-
URI uri = new URI("http://exmaple.com");
134+
public void addingANodeThatIsDownMeansTheGridIsNotReady() throws URISyntaxException {
135+
Capabilities capabilities = new ImmutableCapabilities("cheese", "amsterdam");
136+
URI uri = new URI("https://example.com");
193137

194138
AtomicReference<Availability> isUp = new AtomicReference<>(UP);
195-
196-
Node node = LocalNode.builder(tracer, bus, uri, uri, registrationSecret)
197-
.add(capabilities, new TestSessionFactory(
198-
(id, caps) -> new Session(id, uri, new ImmutableCapabilities(), caps, Instant.now())))
199-
.advanced()
200-
.healthCheck(() -> new HealthCheck.Result(isUp.get(), "TL;DR"))
201-
.build();
139+
Node node = getNode(capabilities, uri, isUp);
202140
distributor.add(node);
203141

204142
waitUntilReady(router, Duration.ofSeconds(5));
205143
isUp.set(DOWN);
206144
waitUntilNotReady(router, Duration.ofSeconds(5));
207145

208-
HttpResponse response = getStatusHttpResponse(router);
209-
assertNotNull(response);
210146
Map<String, Object> status = getStatus(router);
211-
assertFalse((Boolean) status.get("ready"));
147+
assertFalse(status.toString(), (Boolean) status.get("ready"));
212148
}
213149

214150
@Test
215151
public void aNodeThatIsUpAndHasSpareSessionsMeansTheGridIsReady() throws URISyntaxException {
216152
Capabilities capabilities = new ImmutableCapabilities("cheese", "peas");
217-
URI uri = new URI("http://exmaple.com");
218-
219-
AtomicReference<Availability> isUp = new AtomicReference<>(UP);
153+
URI uri = new URI("https://example.com");
220154

221-
Node node = LocalNode.builder(tracer, bus, uri, uri, registrationSecret)
222-
.add(capabilities, new TestSessionFactory((id, caps) -> new Session(id, uri, new ImmutableCapabilities(), caps, Instant.now())))
223-
.advanced()
224-
.healthCheck(() -> new HealthCheck.Result(isUp.get(), "TL;DR"))
225-
.build();
155+
Node node = getNode(capabilities, uri, new AtomicReference<>(UP));
226156
distributor.add(node);
227157

228158
waitUntilReady(router, Duration.ofSeconds(5));
229159
}
230160

231161
@Test
232-
public void shouldListAllNodesTheDistributorIsAwareOf() throws URISyntaxException, IOException {
162+
public void shouldListAllNodesTheDistributorIsAwareOf() throws URISyntaxException {
233163
Capabilities chromeCapabilities = new ImmutableCapabilities("browser", "chrome");
234164
Capabilities firefoxCapabilities = new ImmutableCapabilities("browser", "firefox");
235-
URI firstNodeUri = new URI("http://example1.com");
236-
URI secondNodeUri = new URI("http://example2.com");
165+
URI firstNodeUri = new URI("https://example1.com");
166+
URI secondNodeUri = new URI("https://example2.com");
237167

238168
AtomicReference<Availability> isUp = new AtomicReference<>(UP);
239169

240170
Node firstNode = LocalNode.builder(tracer, bus, firstNodeUri, firstNodeUri, registrationSecret)
241-
.add(chromeCapabilities, new TestSessionFactory((id, caps) -> new Session(id, firstNodeUri, new ImmutableCapabilities(), caps, Instant.now())))
171+
.add(chromeCapabilities, new TestSessionFactory(
172+
(id, caps) -> new Session(id, firstNodeUri, new ImmutableCapabilities(), caps,
173+
Instant.now())))
242174
.advanced()
243175
.healthCheck(() -> new HealthCheck.Result(isUp.get(), "TL;DR"))
244176
.build();
245177

246-
Node secondNode = LocalNode.builder(tracer, bus, secondNodeUri, secondNodeUri, registrationSecret)
247-
.add(firefoxCapabilities, new TestSessionFactory((id, caps) -> new Session(id, secondNodeUri, new ImmutableCapabilities(), caps, Instant.now())))
248-
.advanced()
249-
.healthCheck(() -> new HealthCheck.Result(isUp.get(), "TL;DR"))
250-
.build();
178+
Node
179+
secondNode =
180+
LocalNode.builder(tracer, bus, secondNodeUri, secondNodeUri, registrationSecret)
181+
.add(firefoxCapabilities, new TestSessionFactory(
182+
(id, caps) -> new Session(id, secondNodeUri, new ImmutableCapabilities(), caps,
183+
Instant.now())))
184+
.advanced()
185+
.healthCheck(() -> new HealthCheck.Result(isUp.get(), "TL;DR"))
186+
.build();
251187

252188
distributor.add(firstNode);
253189
distributor.add(secondNode);
254190

255191
waitUntilReady(router, Duration.ofSeconds(5));
256192

257193
Map<String, Object> status = getStatus(router);
258-
List<Map<String,Object>> nodes = (List<Map<String, Object>>) status.get("nodes");
194+
@SuppressWarnings("unchecked")
195+
List<Map<String, Object>> nodes = (List<Map<String, Object>>) status.get("nodes");
259196

260197
assertEquals(2, nodes.size());
261198

262199
String firstNodeId = (String) nodes.get(0).get("id");
263200
String secondNodeId = (String) nodes.get(1).get("id");
264201

265-
assertFalse(firstNodeId.equals(secondNodeId));
202+
assertNotEquals(firstNodeId, secondNodeId);
266203
}
267204

268205
@Test
269-
public void ifNodesHaveSpareSlotsButAlreadyHaveMaxSessionsGridIsReady()
270-
throws URISyntaxException, IOException {
206+
public void ifNodesHaveSpareSlotsButAlreadyHaveMaxSessionsGridIsNotReady()
207+
throws URISyntaxException {
271208
Capabilities chromeCapabilities = new ImmutableCapabilities("browser", "chrome");
272209
Capabilities firefoxCapabilities = new ImmutableCapabilities("browser", "firefox");
273-
URI uri = new URI("http://example.com");
210+
URI uri = new URI("https://example.com");
274211

275212
AtomicReference<Availability> isUp = new AtomicReference<>(UP);
276213

@@ -301,35 +238,43 @@ public void ifNodesHaveSpareSlotsButAlreadyHaveMaxSessionsGridIsReady()
301238
Either<SessionNotCreatedException, CreateSessionResponse> response =
302239
distributor.newSession(sessionRequest);
303240

304-
if (response.isRight()) {
305-
Session session = response.right().getSession();
306-
assertThat(session).isNotNull();
307-
assertTrue(status.toString(), (Boolean) status.get("ready"));
308-
} else {
309-
fail("Session creation failed", response.left());
310-
}
241+
assertTrue(response.isRight());
242+
Session session = response.right().getSession();
243+
assertThat(session).isNotNull();
244+
}
245+
246+
private Node getNode(Capabilities capabilities, URI uri,
247+
AtomicReference<Availability> availability) {
248+
return LocalNode.builder(tracer, bus, uri, uri, registrationSecret)
249+
.add(capabilities, new TestSessionFactory(
250+
(id, caps) -> new Session(id, uri, new ImmutableCapabilities(), caps, Instant.now())))
251+
.advanced()
252+
.healthCheck(() -> new HealthCheck.Result(availability.get(), "TL;DR"))
253+
.build();
311254
}
312255

313-
private Map<String, Object> getStatus(Router router) throws IOException {
256+
private static Map<String, Object> getStatus(Router router) {
314257
HttpResponse response = router.execute(new HttpRequest(GET, "/status"));
315-
Map<String, Object> status = null;
316-
try (Reader reader = reader(response);
317-
JsonInput input = JSON.newInput(reader)) {
318-
input.beginObject();
319-
320-
while (input.hasNext()) {
321-
if ("value".equals(input.nextName())) {
322-
status = input.read(MAP_TYPE);
323-
} else {
324-
input.skipValue();
325-
}
326-
}
327-
}
258+
Map<String, Object> status = Values.get(response, MAP_TYPE);
328259
assertNotNull(status);
329260
return status;
330261
}
331262

332-
private HttpResponse getStatusHttpResponse(Router router) {
333-
return router.execute(new HttpRequest(GET, "/status"));
263+
private static void waitUntilReady(Router router, Duration duration) {
264+
waitUntilStatus(router, duration, Boolean.TRUE);
265+
}
266+
267+
private static void waitUntilNotReady(Router router, Duration duration) {
268+
waitUntilStatus(router, duration, Boolean.FALSE);
269+
}
270+
271+
private static void waitUntilStatus(Router router, Duration duration, Boolean ready) {
272+
new FluentWait<>(router)
273+
.withTimeout(duration)
274+
.pollingEvery(Duration.ofMillis(100))
275+
.until(r -> {
276+
Map<String, Object> status = getStatus(router);
277+
return ready.equals(status.get("ready"));
278+
});
334279
}
335280
}

0 commit comments

Comments
 (0)