Skip to content

Commit 9d24f30

Browse files
wwisserwwisser
committed
refactor: resolve review concerns - shrink try/catch block to cli execution; utilize AutoCloseable iface in tests; make use of StringUtils.isNotEmpty for preconditions
1 parent ef646ba commit 9d24f30

File tree

2 files changed

+60
-48
lines changed

2 files changed

+60
-48
lines changed

‎modules/valkey/src/main/java/org/testcontainers/valkey/ValkeyContainer.java‎

Lines changed: 26 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import com.google.common.base.Preconditions;
44
import lombok.AllArgsConstructor;
55
import lombok.Getter;
6+
import org.apache.commons.lang3.StringUtils;
67
import org.testcontainers.containers.GenericContainer;
78
import org.testcontainers.containers.wait.strategy.Wait;
89
import org.testcontainers.utility.DockerImageName;
@@ -137,12 +138,12 @@ public void start() {
137138
List<String> command = new ArrayList<>();
138139
command.add("valkey-server");
139140

140-
if (configFile != null && !configFile.isEmpty()) {
141+
if (StringUtils.isNotEmpty(configFile)) {
141142
withCopyToContainer(MountableFile.forHostPath(configFile), DEFAULT_CONFIG_FILE);
142143
command.add(DEFAULT_CONFIG_FILE);
143144
}
144145

145-
if (password != null && !password.isEmpty()) {
146+
if (StringUtils.isNotEmpty(password)) {
146147
command.add("--requirepass");
147148
command.add(password);
148149

@@ -158,7 +159,8 @@ public void start() {
158159

159160
if (snapshottingSettings != null) {
160161
command.addAll(
161-
Arrays.asList("--save", snapshottingSettings.getSeconds() + " " + snapshottingSettings.getChangedKeys())
162+
Arrays.asList("--save",
163+
snapshottingSettings.getSeconds() + " " + snapshottingSettings.getChangedKeys())
162164
);
163165
}
164166

@@ -167,7 +169,8 @@ public void start() {
167169
}
168170

169171
if (initialImportScriptFile != null && !initialImportScriptFile.isEmpty()) {
170-
withCopyToContainer(MountableFile.forHostPath(initialImportScriptFile), "/tmp/import.valkey");
172+
withCopyToContainer(MountableFile.forHostPath(initialImportScriptFile),
173+
"/tmp/import.valkey");
171174
withCopyToContainer(MountableFile.forClasspathResource("import.sh"), "/tmp/import.sh");
172175
}
173176

@@ -186,37 +189,37 @@ public int getPort() {
186189
* Executes a command in the Valkey CLI inside the container.
187190
*/
188191
public String executeCli(String cmd, String... flags) {
189-
try {
190-
List<String> args = new ArrayList<>();
191-
args.add("redis-cli");
192-
193-
if (password != null && !password.isEmpty()) {
194-
args.addAll(
195-
username != null && !username.isEmpty()
196-
? Arrays.asList("--user", username, "--pass", password)
197-
: Arrays.asList("--pass", password)
198-
);
199-
}
192+
List<String> args = new ArrayList<>();
193+
args.add("redis-cli");
194+
195+
if (StringUtils.isNotEmpty(password)) {
196+
args.addAll(
197+
StringUtils.isNotEmpty(username)
198+
? Arrays.asList("--user", username, "--pass", password)
199+
: Arrays.asList("--pass", password)
200+
);
201+
}
200202

201-
args.add(cmd);
202-
args.addAll(Arrays.asList(flags));
203+
args.add(cmd);
204+
args.addAll(Arrays.asList(flags));
203205

206+
try {
204207
ExecResult result = execInContainer(args.toArray(new String[0]));
205208
if (result.getExitCode() != 0) {
206209
throw new RuntimeException(result.getStdout() + result.getStderr());
207210
}
208211

209212
return result.getStdout();
210213
} catch (Exception e) {
211-
throw new RuntimeException(e);
214+
throw new RuntimeException("failed to execute CLI command", e);
212215
}
213216
}
214217

215218
public String createConnectionUrl() {
216219
String userInfo = null;
217-
if (username != null && !username.isEmpty() && password != null && !password.isEmpty()) {
220+
if (StringUtils.isNotEmpty(username) && StringUtils.isNotEmpty(password)) {
218221
userInfo = username + ":" + password;
219-
} else if (password != null && !password.isEmpty()) {
222+
} else if (StringUtils.isNotEmpty(password)) {
220223
userInfo = ":" + password;
221224
}
222225

@@ -229,12 +232,13 @@ public String createConnectionUrl() {
229232
}
230233

231234
private void evaluateImportScript() {
232-
if (initialImportScriptFile == null || initialImportScriptFile.isEmpty()) {
235+
if (StringUtils.isEmpty(initialImportScriptFile)) {
233236
return;
234237
}
235238

236239
try {
237-
ExecResult result = execInContainer("/bin/sh", "/tmp/import.sh", password != null ? password : "");
240+
ExecResult result = execInContainer("/bin/sh", "/tmp/import.sh",
241+
password != null ? password : "");
238242

239243
if (result.getExitCode() != 0 || result.getStdout().contains("ERR")) {
240244
throw new RuntimeException("Could not import initial data: " + result.getStdout());

‎modules/valkey/src/test/java/org/testcontainers/valkey/ValkeyContainerTest.java‎

Lines changed: 34 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,8 @@ void shouldWriteAndReadEntry() {
2424
.withSnapshotting(3, 1)
2525
) {
2626
valkeyContainer.start();
27-
JedisPool jedisPool = new JedisPool(valkeyContainer.createConnectionUrl());
28-
29-
try (Jedis jedis = jedisPool.getResource()) {
27+
try (JedisPool jedisPool = new JedisPool(valkeyContainer.createConnectionUrl());
28+
Jedis jedis = jedisPool.getResource()) {
3029
jedis.set("key", "value");
3130
assertThat(jedis.get("key")).isEqualTo("value");
3231
}
@@ -36,20 +35,22 @@ void shouldWriteAndReadEntry() {
3635
@Test
3736
void shouldConfigureServiceWithAuthentication() {
3837
try (
39-
ValkeyContainer valkeyContainer = new ValkeyContainer().withUsername("testuser").withPassword("testpass")
38+
ValkeyContainer valkeyContainer = new ValkeyContainer().withUsername("testuser")
39+
.withPassword("testpass")
4040
) {
4141
valkeyContainer.start();
4242
String url = valkeyContainer.createConnectionUrl();
4343
assertThat(url).contains("testuser:testpass");
4444

45-
JedisPool jedisPool = new JedisPool(url);
46-
try (Jedis jedis = jedisPool.getResource()) {
45+
try (JedisPool jedisPool = new JedisPool(url);
46+
Jedis jedis = jedisPool.getResource()) {
4747
jedis.set("k1", "v2");
4848
assertThat(jedis.get("k1")).isEqualTo("v2");
4949
}
5050
}
5151
}
5252

53+
5354
@Test
5455
void shouldPersistData() {
5556
Path dataDir = tempDir.resolve("valkey-data");
@@ -61,18 +62,21 @@ void shouldPersistData() {
6162
.withSnapshotting(1, 1)
6263
) {
6364
valkeyContainer.start();
64-
JedisPool jedisPool = new JedisPool(valkeyContainer.createConnectionUrl());
6565

66-
try (Jedis jedis = jedisPool.getResource()) {
66+
String containerConnectionUrl = valkeyContainer.createConnectionUrl();
67+
try (JedisPool jedisPool = new JedisPool(containerConnectionUrl);
68+
Jedis jedis = jedisPool.getResource()) {
6769
jedis.set("persistKey", "persistValue");
6870
}
6971

7072
valkeyContainer.stop();
71-
try (ValkeyContainer restarted = new ValkeyContainer().withPersistenceVolume(dataDir.toString())) {
73+
try (ValkeyContainer restarted = new ValkeyContainer().withPersistenceVolume(
74+
dataDir.toString())) {
7275
restarted.start();
73-
JedisPool restartedPool = new JedisPool(restarted.createConnectionUrl());
76+
String connectionUrl = restarted.createConnectionUrl();
7477

75-
try (Jedis jedis = restartedPool.getResource()) {
78+
try (JedisPool restartedPool = new JedisPool(connectionUrl);
79+
Jedis jedis = restartedPool.getResource()) {
7680
assertThat(jedis.get("persistKey")).isEqualTo("persistValue");
7781
}
7882
}
@@ -83,11 +87,13 @@ void shouldPersistData() {
8387
void shouldInitializeDatabaseWithPayload() throws Exception {
8488
Path importFile = Paths.get(getClass().getResource("/initData.valkey").toURI());
8589

86-
try (ValkeyContainer valkeyContainer = new ValkeyContainer().withInitialData(importFile.toString())) {
90+
try (ValkeyContainer valkeyContainer = new ValkeyContainer().withInitialData(
91+
importFile.toString())) {
8792
valkeyContainer.start();
88-
JedisPool jedisPool = new JedisPool(valkeyContainer.createConnectionUrl());
93+
String connectionUrl = valkeyContainer.createConnectionUrl();
8994

90-
try (Jedis jedis = jedisPool.getResource()) {
95+
try (JedisPool jedisPool = new JedisPool(
96+
connectionUrl); Jedis jedis = jedisPool.getResource()) {
9197
assertThat(jedis.get("key1")).isEqualTo("value1");
9298
assertThat(jedis.get("key2")).isEqualTo("value2");
9399
}
@@ -109,12 +115,13 @@ void shouldExecuteContainerCmdAndReturnResult() {
109115
void shouldMountValkeyConfigToContainer() throws Exception {
110116
Path configFile = Paths.get(getClass().getResource("/valkey.conf").toURI());
111117

112-
try (ValkeyContainer valkeyContainer = new ValkeyContainer().withConfigFile(configFile.toString())) {
118+
try (ValkeyContainer valkeyContainer = new ValkeyContainer().withConfigFile(
119+
configFile.toString())) {
113120
valkeyContainer.start();
114121

115-
JedisPool jedisPool = new JedisPool(valkeyContainer.createConnectionUrl());
116-
117-
try (Jedis jedis = jedisPool.getResource()) {
122+
String connectionUrl = valkeyContainer.createConnectionUrl();
123+
try (JedisPool jedisPool = new JedisPool(connectionUrl);
124+
Jedis jedis = jedisPool.getResource()) {
118125
String maxMemory = jedis.configGet("maxmemory").get("maxmemory");
119126

120127
assertThat(maxMemory).isEqualTo("2097152");
@@ -124,13 +131,14 @@ void shouldMountValkeyConfigToContainer() throws Exception {
124131

125132
@Test
126133
void shouldValidateSnapshottingConfiguration() {
127-
ValkeyContainer container = new ValkeyContainer();
128-
assertThatThrownBy(() -> container.withSnapshotting(0, 10))
129-
.isInstanceOf(IllegalArgumentException.class)
130-
.hasMessageContaining("seconds must be greater than 0");
131-
132-
assertThatThrownBy(() -> container.withSnapshotting(10, 0))
133-
.isInstanceOf(IllegalArgumentException.class)
134-
.hasMessageContaining("changedKeys must be non-negative");
134+
try (ValkeyContainer container = new ValkeyContainer()) {
135+
assertThatThrownBy(() -> container.withSnapshotting(0, 10))
136+
.isInstanceOf(IllegalArgumentException.class)
137+
.hasMessageContaining("seconds must be greater than 0");
138+
139+
assertThatThrownBy(() -> container.withSnapshotting(10, 0))
140+
.isInstanceOf(IllegalArgumentException.class)
141+
.hasMessageContaining("changedKeys must be non-negative");
142+
}
135143
}
136144
}

0 commit comments

Comments
 (0)