dcapwell commented on PR #3536:
URL: https://github.com/apache/cassandra/pull/3536#issuecomment-2341748443
I would recommend adding the following patch as it improves the test
coverage. In `CQLTester.Fuzzed` we will create random yaml configs and
includes this layer. The patch makes sure to test with randomized ordering
(and finds the bug reported), and includes a fuzz test showing your changes are
stable and work as expected
```
diff --git a/test/unit/org/apache/cassandra/schema/MemtableParamsTest.java
b/test/unit/org/apache/cassandra/schema/MemtableParamsTest.java
index 7db5204a9f..d12dcb556f 100644
--- a/test/unit/org/apache/cassandra/schema/MemtableParamsTest.java
+++ b/test/unit/org/apache/cassandra/schema/MemtableParamsTest.java
@@ -20,7 +20,11 @@ package org.apache.cassandra.schema;
import java.util.Map;
+import accord.utils.Gen;
import com.google.common.collect.ImmutableMap;
+import org.apache.cassandra.config.Config;
+import org.apache.cassandra.utils.ConfigGenBuilder;
+import org.assertj.core.api.Assertions;
import org.junit.Assert;
import org.junit.Test;
@@ -29,172 +33,187 @@ import org.apache.cassandra.config.ParameterizedClass;
import org.apache.cassandra.db.memtable.SkipListMemtableFactory;
import org.apache.cassandra.exceptions.ConfigurationException;
+import static accord.utils.Property.qt;
+import static org.apache.cassandra.config.YamlConfigurationLoader.fromMap;
import static org.junit.Assert.assertEquals;
-public class MemtableParamsTest
-{
+public class MemtableParamsTest {
static final ParameterizedClass DEFAULT =
SkipListMemtableFactory.CONFIGURATION;
@Test
- public void testDefault()
- {
+ public void testDefault() {
Map<String, ParameterizedClass> map =
MemtableParams.expandDefinitions(ImmutableMap.of());
assertEquals(ImmutableMap.of("default", DEFAULT), map);
}
@Test
- public void testDefaultRemapped()
- {
+ public void testDefaultRemapped() {
Map<String, ParameterizedClass> map =
MemtableParams.expandDefinitions
- (
- ImmutableMap.of("remap", new InheritingClass("default", null,
null))
- );
+ (
+ ImmutableMap.of("remap", new
InheritingClass("default", null, null))
+ );
assertEquals(ImmutableMap.of("default", DEFAULT,
- "remap", DEFAULT),
- map);
+ "remap", DEFAULT),
+ map);
}
@Test
- public void testOne()
- {
+ public void testOne() {
final InheritingClass one = new InheritingClass(null, "SkipList",
null);
Map<String, ParameterizedClass> map =
MemtableParams.expandDefinitions(ImmutableMap.of("one", one));
assertEquals(ImmutableMap.of("default", DEFAULT,
- "one", one),
- map);
+ "one", one),
+ map);
}
@Test
- public void testExtends()
- {
+ public void testExtends() {
final InheritingClass one = new InheritingClass(null, "SkipList",
null);
Map<String, ParameterizedClass> map =
MemtableParams.expandDefinitions
- (
- ImmutableMap.of("one", one,
- "two", new InheritingClass("one",
- null,
-
ImmutableMap.of("extra", "value")))
- );
+ (
+ ImmutableMap.of("one", one,
+ "two", new InheritingClass("one",
+ null,
+ ImmutableMap.of("extra", "value")))
+ );
assertEquals(ImmutableMap.of("default", DEFAULT,
- "one", one,
- "two", new
ParameterizedClass("SkipList",
-
ImmutableMap.of("extra", "value"))),
- map);
+ "one", one,
+ "two", new ParameterizedClass("SkipList",
+ ImmutableMap.of("extra", "value"))),
+ map);
}
@Test
- public void testExtendsReplace()
- {
+ public void testExtendsReplace() {
final InheritingClass one = new InheritingClass(null,
- "SkipList",
-
ImmutableMap.of("extra", "valueOne"));
+ "SkipList",
+ ImmutableMap.of("extra", "valueOne"));
Map<String, ParameterizedClass> map =
MemtableParams.expandDefinitions
- (
- ImmutableMap.of("one", one,
- "two", new InheritingClass("one",
- null,
-
ImmutableMap.of("extra", "value")))
- );
+ (
+ ImmutableMap.of("one", one,
+ "two", new InheritingClass("one",
+ null,
+ ImmutableMap.of("extra", "value")))
+ );
assertEquals(ImmutableMap.of("default", DEFAULT,
- "one", one,
- "two", new
ParameterizedClass("SkipList",
-
ImmutableMap.of("extra", "value"))),
- map);
+ "one", one,
+ "two", new ParameterizedClass("SkipList",
+ ImmutableMap.of("extra", "value"))),
+ map);
}
@Test
- public void testDoubleExtends()
- {
+ public void testDoubleExtends() {
final InheritingClass one = new InheritingClass(null, "SkipList",
null);
Map<String, ParameterizedClass> map =
MemtableParams.expandDefinitions
- (
- ImmutableMap.of("one", one,
- "two", new InheritingClass("one",
- null,
-
ImmutableMap.of("param", "valueTwo",
-
"extra", "value")),
- "three", new InheritingClass("two",
- "OtherClass",
-
ImmutableMap.of("param", "valueThree",
-
"extraThree", "three")))
- );
+ (
+ ImmutableMap.of("one", one,
+ "two", new InheritingClass("one",
+ null,
+ ImmutableMap.of("param", "valueTwo",
+ "extra", "value")),
+ "three", new InheritingClass("two",
+ "OtherClass",
+ ImmutableMap.of("param",
"valueThree",
+ "extraThree", "three")))
+ );
assertEquals(ImmutableMap.of("default", DEFAULT,
- "one", one,
- "two", new
ParameterizedClass("SkipList",
-
ImmutableMap.of("param", "valueTwo",
-
"extra", "value")),
- "three", new
ParameterizedClass("OtherClass",
-
ImmutableMap.of("param", "valueThree",
-
"extra", "value",
-
"extraThree", "three"))),
- map);
+ "one", one,
+ "two", new ParameterizedClass("SkipList",
+ ImmutableMap.of("param", "valueTwo",
+ "extra", "value")),
+ "three", new ParameterizedClass("OtherClass",
+ ImmutableMap.of("param", "valueThree",
+ "extra", "value",
+ "extraThree", "three"))),
+ map);
}
@Test
- public void testInvalidExtends()
- {
+ public void testInvalidExtends() {
final InheritingClass one = new InheritingClass(null, "SkipList",
null);
- try
- {
+ try {
Map<String, ParameterizedClass> map =
MemtableParams.expandDefinitions
- (
- ImmutableMap.of("two", new InheritingClass("one",
- null,
-
ImmutableMap.of("extra", "value")),
- "one", one)
- );
+ (
+ ImmutableMap.of("two", new
InheritingClass("one",
+ null,
+ ImmutableMap.of("extra",
"value")),
+ "one", one)
+ );
Assert.fail("Expected exception.");
- }
- catch (ConfigurationException e)
- {
+ } catch (ConfigurationException e) {
// expected
}
}
@Test
- public void testInvalidSelfExtends()
- {
- try
- {
+ public void testInvalidSelfExtends() {
+ try {
Map<String, ParameterizedClass> map =
MemtableParams.expandDefinitions
- (
- ImmutableMap.of("one", new InheritingClass("one",
- null,
-
ImmutableMap.of("extra", "value")))
- );
+ (
+ ImmutableMap.of("one", new
InheritingClass("one",
+ null,
+ ImmutableMap.of("extra", "value")))
+ );
Assert.fail("Expected exception.");
- }
- catch (ConfigurationException e)
- {
+ } catch (ConfigurationException e) {
// expected
}
}
@Test
- public void testReplaceDefault()
- {
+ public void testReplaceDefault() {
final InheritingClass one = new InheritingClass(null,
- "SkipList",
-
ImmutableMap.of("extra", "valueOne"));
+ "SkipList",
+ ImmutableMap.of("extra", "valueOne"));
Map<String, ParameterizedClass> map =
MemtableParams.expandDefinitions(ImmutableMap.of("default", one));
assertEquals(ImmutableMap.of("default", one), map);
}
@Test
- public void testDefaultExtends()
- {
+ public void testDefaultExtends() {
final InheritingClass one = new InheritingClass(null,
- "SkipList",
-
ImmutableMap.of("extra", "valueOne"));
+ "SkipList",
+ ImmutableMap.of("extra", "valueOne"));
Map<String, ParameterizedClass> map =
MemtableParams.expandDefinitions
- (
- ImmutableMap.of("one", one,
- "default", new InheritingClass("one", null,
ImmutableMap.of()))
- );
+ (
+ ImmutableMap.of("one", one,
+ "default", new InheritingClass("one", null,
ImmutableMap.of()))
+ );
assertEquals(ImmutableMap.of("one", one,
- "default", one),
- map);
+ "default", one),
+ map);
}
// Note: The factories constructed from these parameters are tested in
the CreateTest and AlterTest.
+
+ @Test
+ public void testExpandDefinitions() {
+ Gen<Map<String, InheritingClass>> gen = new ConfigGenBuilder()
+ .buildMemtable()
+ .map(m -> fromMap(m, Config.class))
+ .map(c -> c.memtable.configurations);
+ qt().forAll(gen).check(configs -> {
+ Map<String, ParameterizedClass> result =
MemtableParams.expandDefinitions(configs);
+
Assertions.assertThat(result.keySet()).isEqualTo(configs.keySet());
+ for (Map.Entry<String, InheritingClass> e : configs.entrySet())
{
+ if (e.getValue().inherits == null) continue;
+ ParameterizedClass parent =
result.get(e.getValue().inherits);
+ ParameterizedClass src = result.get(e.getKey());
+
Assertions.assertThat(src.class_name).isEqualTo(parent.class_name);
+
Assertions.assertThat(src.parameters).isEqualTo(parent.parameters);
+ }
+ });
+ }
+
+ @Test
+ public void testInheritsUnresolved() {
+ MemtableParams.expandDefinitions(Map.of("a", inherits("b"),
+ "b", inherits("a")));
+ }
+
+ private static InheritingClass inherits(String name)
+ {
+ return new InheritingClass(name, null, null);
+ }
}
diff --git a/test/unit/org/apache/cassandra/utils/ConfigGenBuilder.java
b/test/unit/org/apache/cassandra/utils/ConfigGenBuilder.java
index e0b89e767c..f339e42522 100644
--- a/test/unit/org/apache/cassandra/utils/ConfigGenBuilder.java
+++ b/test/unit/org/apache/cassandra/utils/ConfigGenBuilder.java
@@ -18,9 +18,7 @@
package org.apache.cassandra.utils;
-import java.util.LinkedHashMap;
-import java.util.Map;
-import java.util.Objects;
+import java.util.*;
import java.util.concurrent.TimeUnit;
import javax.annotation.Nullable;
@@ -148,24 +146,65 @@ public class ConfigGenBuilder
config.put("commitlog_disk_access_mode",
commitLogDiskAccessModeGen.next(rs));
}
- private void updateConfigMemtable(RandomSource rs, Map<String, Object>
config)
+ public Gen<Map<String, Object>> buildMemtable()
+ {
+ return rs -> {
+ LinkedHashMap<String, Object> config = new LinkedHashMap<>();
+ updateConfigMemtable(rs, config);
+ return config;
+ };
+ }
+
+ private enum MemtableConfigShape { Single, Simple, Nested}
+ public void updateConfigMemtable(RandomSource rs, Map<String, Object>
config)
{
config.put("memtable_allocation_type",
memtableAllocationTypeGen.next(rs));
Memtable defaultMemtable = memtableGen.next(rs);
- Map<String, Map<String, Object>> memtables = new LinkedHashMap<>();
- if (rs.nextBoolean())
+ LinkedHashMap<String, Map<String, Object>> memtables = new
LinkedHashMap<>();
+ switch (rs.pick(MemtableConfigShape.values()))
{
- // use inherits
- for (Memtable m : Memtable.values())
- memtables.put(m.name(), createConfig(m).next(rs));
- memtables.put("default", ImmutableMap.of("inherits",
defaultMemtable.name()));
- }
- else
- {
- // define inline
- memtables.put("default",
createConfig(defaultMemtable).next(rs));
+ case Single:
+ // define inline
+ memtables.put("default",
createConfig(defaultMemtable).next(rs));
+ break;
+ case Simple:
+ // use inherits
+ for (Memtable m : Memtable.values())
+ memtables.put(m.name(), createConfig(m).next(rs));
+ memtables.put("default", ImmutableMap.of("inherits",
defaultMemtable.name()));
+ break;
+ case Nested:
+ {
+ for (Memtable m : Memtable.values())
+ {
+ int levels = rs.nextInt(0, 4);
+ String prev = m.name() + "_base";
+ memtables.put(prev, createConfig(m).next(rs));
+ for (int i = 0; i < levels; i++)
+ {
+ String next = m.name() + '_' + i;
+ memtables.put(next, ImmutableMap.of("inherits",
prev));
+ prev = next;
+ }
+ memtables.put(m.name(), ImmutableMap.of("inherits",
prev));
+ }
+ memtables.put("default", ImmutableMap.of("inherits",
defaultMemtable.name()));
+ }
+ break;
+ default:
+ throw new UnsupportedOperationException();
}
- config.put("memtable", ImmutableMap.of("configurations",
memtables));
+ config.put("memtable", ImmutableMap.of("configurations",
scrable(rs, memtables)));
+ }
+
+ private static <K, V> LinkedHashMap<K, V> scrable(RandomSource rs,
LinkedHashMap<K, V> map)
+ {
+ List<K> keys = new ArrayList<>(map.keySet());
+ Collections.shuffle(keys, rs.asJdkRandom());
+ LinkedHashMap<K, V> copy = new LinkedHashMap<>();
+ for (K key : keys)
+ copy.put(key, map.get(key));
+ return copy;
}
private static Gen<Map<String, Object>> createConfig(Memtable type)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
