Skip to content

Commit c0a3b39

Browse files
mach6mach6
committed
More tests and tweaks for RegistrationRequest
1 parent 9f2c118 commit c0a3b39

2 files changed

Lines changed: 156 additions & 59 deletions

File tree

‎java/server/src/org/openqa/grid/common/RegistrationRequest.java‎

Lines changed: 50 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -65,16 +65,22 @@ public RegistrationRequest() {
6565

6666
/**
6767
* Create a new registration request using the supplied {@link GridNodeConfiguration}
68-
* @param configuration the {@link GridNodeConfiguration} to use
68+
*
69+
* @param configuration the {@link GridNodeConfiguration} to use. Internally calls {@code new
70+
* GridNodeConfiguration()} if a {@code null} value is provided since a
71+
* request without configuration is not valid.
6972
*/
7073
public RegistrationRequest(GridNodeConfiguration configuration) {
7174
this(configuration, null, null);
7275
}
7376

7477
/**
7578
* Create a new registration request using the supplied {@link GridNodeConfiguration}, and name
76-
* @param configuration the {@link GridNodeConfiguration} to use
77-
* @param name the name for the remote
79+
*
80+
* @param configuration the {@link GridNodeConfiguration} to use. Internally calls {@code new
81+
* GridNodeConfiguration()} if a {@code null} value is provided since a
82+
* request without configuration is not valid.
83+
* @param name the name for the remote
7884
*/
7985
public RegistrationRequest(GridNodeConfiguration configuration, String name) {
8086
this(configuration, name, null);
@@ -83,12 +89,15 @@ public RegistrationRequest(GridNodeConfiguration configuration, String name) {
8389
/**
8490
* Create a new registration request using the supplied {@link GridNodeConfiguration}, name, and
8591
* description
86-
* @param configuration the {@link GridNodeConfiguration} to use
87-
* @param name the name for the remote
88-
* @param description the description for the remote host
92+
*
93+
* @param configuration the {@link GridNodeConfiguration} to use. Internally calls {@code new
94+
* GridNodeConfiguration()} if a {@code null} value is provided since a
95+
* request without configuration is not valid.
96+
* @param name the name for the remote
97+
* @param description the description for the remote host
8998
*/
9099
public RegistrationRequest(GridNodeConfiguration configuration, String name, String description) {
91-
this.configuration = configuration;
100+
this.configuration = (configuration == null) ? new GridNodeConfiguration() : configuration;
92101
this.name = name;
93102
this.description = description;
94103

@@ -155,55 +164,57 @@ public static RegistrationRequest fromJson(String json) throws JsonSyntaxExcepti
155164
}
156165

157166
/**
158-
* Build a RegistrationRequest. This is different than {@code new RegistrationRequest()} because
159-
* it will "fixup" the resulting RegistrationRequest before returning the result
167+
* Build a RegistrationRequest.
160168
* @return
161169
*/
162170
public static RegistrationRequest build() {
163-
return RegistrationRequest.build(null, null, null);
171+
return RegistrationRequest.build(new GridNodeConfiguration(), null, null);
164172
}
165173

166174
/**
167175
* Build a RegistrationRequest from the provided {@link GridNodeConfiguration}. This is different
168-
* than {@code new RegistrationRequest(GridNodeConfiguration)} because it will merge any
169-
* specified {@link GridNodeConfiguration#nodeConfigFile} onto the provided configuration and it
170-
* will "fixup" the resulting RegistrationRequest before returning the result
171-
* @param configuration the {@link GridNodeConfiguration} to use
172-
* @return
176+
* than {@code new RegistrationRequest(GridNodeConfiguration)} because it will first load any
177+
* specified {@link GridNodeConfiguration#nodeConfigFile} and then merge the provided
178+
* configuration onto it.
179+
*
180+
* @param configuration the {@link GridNodeConfiguration} to use. Internally calls {@code new
181+
* GridNodeConfiguration()} if a {@code null} value is provided since a
182+
* request without configuration is not valid.
173183
*/
174184
public static RegistrationRequest build(GridNodeConfiguration configuration) {
175185
return RegistrationRequest.build(configuration, null, null);
176186
}
177187

178188
/**
179-
* Build a RegistrationRequest from the provided {@link GridNodeConfiguration}, use the provided name.
180-
* This is different than {@code new RegistrationRequest(GridNodeConfiguration, String)} because it
181-
* will merge any specified {@link GridNodeConfiguration#nodeConfigFile} onto the provided
182-
* configuration and it will "fixup" the resulting RegistrationRequest before returning the result
183-
* @param configuration the {@link GridNodeConfiguration} to use
184-
* @param name the name for the remote
185-
* @return
189+
* Build a RegistrationRequest from the provided {@link GridNodeConfiguration}, use the provided
190+
* name. This is different than {@code new RegistrationRequest(GridNodeConfiguration, String)}
191+
* because it will first load any specified {@link GridNodeConfiguration#nodeConfigFile} and then
192+
* merge the provided configuration onto it.
193+
*
194+
* @param configuration the {@link GridNodeConfiguration} to use. Internally calls {@code new
195+
* GridNodeConfiguration()} if a {@code null} value is provided since a
196+
* request without configuration is not valid.
197+
* @param name the name for the remote
186198
*/
187199
public static RegistrationRequest build(GridNodeConfiguration configuration, String name) {
188200
return RegistrationRequest.build(configuration, name, null);
189201
}
190202

191203
/**
192-
* Build a RegistrationRequest from the provided {@link GridNodeConfiguration}, use the provided name
193-
* and description. This is different than
194-
* {@code new RegistrationRequest(GridNodeConfiguration, String, String)} because it will merge any
195-
* specified {@link GridNodeConfiguration#nodeConfigFile} onto the provided configuration and it
196-
* will "fixup" the resulting RegistrationRequest before returning the result
197-
* @param configuration the {@link GridNodeConfiguration} to use
198-
* @param name the name for the remote
199-
* @param description the description for the remote host
200-
* @return
204+
* Build a RegistrationRequest from the provided {@link GridNodeConfiguration}, use the provided
205+
* name and description. This is different than {@code new RegistrationRequest(GridNodeConfiguration,
206+
* String, String)} because it will first load any specified {@link
207+
* GridNodeConfiguration#nodeConfigFile} and then merge the provided configuration onto it.
208+
*
209+
* @param configuration the {@link GridNodeConfiguration} to use. Internally calls {@code new
210+
* GridNodeConfiguration()} if a {@code null} value is provided since a
211+
* request without configuration is not valid.
212+
* @param name the name for the remote
213+
* @param description the description for the remote host
201214
*/
202215
public static RegistrationRequest build(GridNodeConfiguration configuration, String name, String description) {
203-
GridNodeConfiguration pendingConfiguration = (configuration == null) ?
204-
new GridNodeConfiguration() : configuration;
205-
206-
RegistrationRequest pendingRequest = new RegistrationRequest(pendingConfiguration, name, description);
216+
RegistrationRequest pendingRequest = new RegistrationRequest(configuration, name, description);
217+
GridNodeConfiguration pendingConfiguration = pendingRequest.configuration;
207218

208219
if (pendingConfiguration.nodeConfigFile != null) {
209220
pendingRequest.configuration = GridNodeConfiguration.loadFromJSON(pendingConfiguration.nodeConfigFile);
@@ -227,6 +238,10 @@ public static RegistrationRequest build(GridNodeConfiguration configuration, Str
227238
}
228239

229240
private void fixUpCapabilities() {
241+
if (configuration.capabilities == null) {
242+
return; // assumes the caller set it/wants it this way
243+
}
244+
230245
Platform current = Platform.getCurrent();
231246
for (DesiredCapabilities cap : configuration.capabilities) {
232247
if (cap.getPlatform() == null) {

‎java/server/test/org/openqa/grid/common/RegistrationRequestTest.java‎

Lines changed: 106 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121
import static org.junit.Assert.assertNotNull;
2222
import static org.junit.Assert.assertNull;
2323
import static org.junit.Assert.assertTrue;
24+
import static org.junit.Assert.assertSame;
25+
import static org.junit.Assert.assertNotSame;
2426

2527
import com.beust.jcommander.JCommander;
2628

@@ -151,44 +153,124 @@ public void validateWithException() {
151153
*/
152154
@Test
153155
public void testBuildWithConfiguration() {
154-
GridNodeConfiguration config = new GridNodeConfiguration();
155-
config.maxSession = 50;
156-
config.timeout = 10;
156+
GridNodeConfiguration actualConfig = new GridNodeConfiguration();
157+
actualConfig.maxSession = 50;
158+
actualConfig.timeout = 10;
159+
actualConfig.host = "dummyhost";
160+
actualConfig.port = 1234;
161+
actualConfig.capabilities.set(0, DesiredCapabilities.operaBlink());
162+
actualConfig.nodeConfigFile = GridNodeConfiguration.DEFAULT_NODE_CONFIG_FILE;
163+
164+
RegistrationRequest req = RegistrationRequest.build(actualConfig);
165+
assertConstruction(req);
166+
167+
// we should get a new config object ref back, since the nodeConfigFile was processed
168+
assertNotSame(req.getConfiguration(), actualConfig);
169+
// reset to new config which build() produced
170+
actualConfig = req.getConfiguration();
171+
172+
// make sure the nodeConfigFile was processed by ensuring that it now is null;
173+
assertNull(actualConfig.nodeConfigFile);
174+
175+
// make sure the first capability is for operaBlink
176+
assertEquals(DesiredCapabilities.operaBlink().getBrowserName(),
177+
actualConfig.capabilities.get(0).getBrowserName());
178+
179+
// make sure this merge protected value was preserved, then remove it for the final assert
180+
assertEquals("dummyhost", actualConfig.host);
181+
actualConfig.host = null;
182+
// make sure this merge protected value was preserved, then reset it for the final assert
183+
assertEquals(1234, actualConfig.port.intValue());
184+
actualConfig.port = 5555;
185+
186+
// merge actualConfig onto it.. which is what build(config) should do
187+
GridNodeConfiguration expectedConfig = new GridNodeConfiguration();
188+
expectedConfig.merge(actualConfig);
157189

158-
RegistrationRequest req = RegistrationRequest.build(config);
190+
assertEquals(expectedConfig.toString(), actualConfig.toString());
191+
}
159192

160-
// should have the default capabilities
161-
assertEquals(3, req.getConfiguration().capabilities.size());
193+
/**
194+
* Tests that new RegistrationRequest(config) performs as expected
195+
*/
196+
@Test
197+
public void testConstructorWithConfiguration() {
198+
GridNodeConfiguration actualConfig = new GridNodeConfiguration();
199+
actualConfig.host = "dummyhost";
200+
RegistrationRequest req = new RegistrationRequest(actualConfig);
201+
assertConstruction(req);
202+
// make sure the provided config was used.
203+
assertSame(req.getConfiguration(), actualConfig);
204+
}
162205

163-
// host is "fixed up" by the .build(config) call
164-
// verify this happened and remove it for the final assert.
165-
assertNotNull(req.getConfiguration().host);
166-
req.getConfiguration().host = null;
206+
/**
207+
* Tests that RegistrationRequest.build() performs as expected
208+
*/
209+
@Test
210+
public void testBuild() {
211+
assertConstruction(RegistrationRequest.build());
212+
}
167213

168-
// capabilities are seeded from the default node config and "fixed up" by the .build(config)
169-
// call. They should now contain a "platform".
170-
// verify this and remove them for the final assert
171-
assertTrue(req.getConfiguration().capabilities.size() > 0);
172-
for (DesiredCapabilities capabilities : req.getConfiguration().capabilities) {
173-
assertNotNull(capabilities.getPlatform());
174-
assertNotNull(capabilities.getCapability("seleniumProtocol"));
175-
}
214+
/**
215+
* Tests that RegistrationRequest.build(null) performs as expected
216+
*/
217+
@Test
218+
public void testBuildWithNullConfiguration() {
219+
assertConstruction(RegistrationRequest.build(null));
220+
}
176221

177-
GridNodeConfiguration expectedConfig = new GridNodeConfiguration();
178-
expectedConfig.merge(config);
222+
/**
223+
* Tests that new RegistrationRequest() performs as expected
224+
*/
225+
@Test
226+
public void testNoArgConstructor() {
227+
assertConstruction(new RegistrationRequest());
228+
}
179229

180-
assertEquals(expectedConfig.toString(), req.getConfiguration().toString());
230+
/**
231+
* Tests that new RegistrationRequest(null) performs as expected
232+
*/
233+
@Test
234+
public void testConstructorWithNullConfiguration() {
235+
assertConstruction(new RegistrationRequest(null));
181236
}
182237

183238
/**
184-
* Tests that RegistrationRequest.build() performs as expected
239+
* Should not result in any NPE during the internal call to fixUpCapabilities
185240
*/
186241
@Test
187-
public void testBuild() {
188-
RegistrationRequest req = RegistrationRequest.build();
242+
public void testConstructorWithConfigurationAndNullCapabilities() {
243+
GridNodeConfiguration config = new GridNodeConfiguration();
244+
config.capabilities = null;
245+
RegistrationRequest req = new RegistrationRequest(config);
246+
assertNull(req.getConfiguration().capabilities);
247+
}
248+
249+
/**
250+
* Should not result in any NPE during the internal call to fixUpCapabilities
251+
*/
252+
@Test
253+
public void testBuildWithConfigurationAndNullCapabilities() {
254+
GridNodeConfiguration config = new GridNodeConfiguration();
255+
config.capabilities = null;
256+
RegistrationRequest req = RegistrationRequest.build(config);
257+
assertNull(req.getConfiguration().capabilities);
258+
}
259+
260+
private void assertConstruction(RegistrationRequest req) {
189261
assertNotNull(req);
190262
assertNotNull(req.getConfiguration());
191263
assertNull(req.getName());
192264
assertNull(req.getDescription());
265+
// fixUpHost should have been internally called
266+
assertNotNull(req.getConfiguration().host);
267+
assertNotNull(req.getConfiguration().capabilities);
268+
// should have the default capabilities
269+
// fixUpCapabilities should have been internally called
270+
assertEquals(3, req.getConfiguration().capabilities.size());
271+
for (DesiredCapabilities capabilities : req.getConfiguration().capabilities) {
272+
assertNotNull(capabilities.getPlatform());
273+
assertNotNull(capabilities.getCapability("seleniumProtocol"));
274+
}
193275
}
194276
}

0 commit comments

Comments
 (0)