You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2020/12/01 14:12:21 UTC
[GitHub] [cassandra] blerer commented on a change in pull request #811: CASSANDRA-16249: Fix the missing bb position in getUnsignedShort
blerer commented on a change in pull request #811:
URL: https://github.com/apache/cassandra/pull/811#discussion_r533418984
##########
File path: test/unit/org/apache/cassandra/db/marshal/ValueAccessorTest.java
##########
@@ -20,75 +20,95 @@
import java.io.IOException;
import java.nio.ByteBuffer;
-import java.util.Arrays;
import java.util.Random;
+import java.util.function.Function;
+import java.util.function.Supplier;
-import com.google.common.primitives.Shorts;
+import org.apache.commons.lang3.tuple.ImmutablePair;
+import org.apache.commons.lang3.tuple.ImmutableTriple;
import org.junit.Assert;
+import org.junit.BeforeClass;
import org.junit.Test;
import org.apache.cassandra.io.util.DataOutputBuffer;
-import org.apache.cassandra.utils.ByteArrayUtil;
import org.apache.cassandra.utils.ByteBufferUtil;
import org.assertj.core.api.Assertions;
import org.quicktheories.core.Gen;
import org.quicktheories.generators.SourceDSL;
import static org.apache.cassandra.db.marshal.ValueAccessors.ACCESSORS;
+import static org.apache.cassandra.utils.ByteArrayUtil.bytesToHex;
+import static org.apache.cassandra.utils.ByteBufferUtil.bytesToHex;
import static org.quicktheories.QuickTheory.qt;
+import static org.quicktheories.generators.SourceDSL.arbitrary;
+import static org.quicktheories.generators.SourceDSL.doubles;
+import static org.quicktheories.generators.SourceDSL.integers;
public class ValueAccessorTest
{
- private static byte[] randomBytes(int minSize, int maxSize, Random random)
+ final static Supplier<Gen<Integer>> leftPadding = () -> integers().between(0, 2);
+ final static Supplier<Gen<Integer>> rightPadding = () -> integers().between(0, 2);
+ final static Supplier<Gen<ValueAccessor>> accessors = () -> SourceDSL.arbitrary().pick(ACCESSORS);
+ final static Supplier<Gen<Function>> paddings = () -> leftPadding.get().zip(rightPadding.get(), (l, r) -> v -> maybePad(v, l, r));
+ final static byte[] bytes = new byte[200];
+
+ @BeforeClass
+ public static void beforeClass()
{
- int size = random.nextInt(maxSize - minSize + 1) + minSize;
- byte[] bytes = new byte[size];
- random.nextBytes(bytes);
- return bytes;
+ new Random(0).nextBytes(bytes);
+ }
+
+ /**
+ * This method should be used to test values to be processed by {@link ValueAccessor}
+ * in order to make sure we do not assume position == 0 in the underlying {@link ByteBuffer}
+ */
+ public static <V> V maybePad(V value, int lPad, int rPad)
Review comment:
I do not think that padding on the right bring any value. If some of the tests were exceeding the `limit` they would fail even without the right padding.
##########
File path: test/unit/org/apache/cassandra/db/marshal/ValueAccessorTest.java
##########
@@ -20,75 +20,95 @@
import java.io.IOException;
import java.nio.ByteBuffer;
-import java.util.Arrays;
import java.util.Random;
+import java.util.function.Function;
+import java.util.function.Supplier;
-import com.google.common.primitives.Shorts;
+import org.apache.commons.lang3.tuple.ImmutablePair;
+import org.apache.commons.lang3.tuple.ImmutableTriple;
import org.junit.Assert;
+import org.junit.BeforeClass;
import org.junit.Test;
import org.apache.cassandra.io.util.DataOutputBuffer;
-import org.apache.cassandra.utils.ByteArrayUtil;
import org.apache.cassandra.utils.ByteBufferUtil;
import org.assertj.core.api.Assertions;
import org.quicktheories.core.Gen;
import org.quicktheories.generators.SourceDSL;
import static org.apache.cassandra.db.marshal.ValueAccessors.ACCESSORS;
+import static org.apache.cassandra.utils.ByteArrayUtil.bytesToHex;
+import static org.apache.cassandra.utils.ByteBufferUtil.bytesToHex;
import static org.quicktheories.QuickTheory.qt;
+import static org.quicktheories.generators.SourceDSL.arbitrary;
+import static org.quicktheories.generators.SourceDSL.doubles;
+import static org.quicktheories.generators.SourceDSL.integers;
public class ValueAccessorTest
{
- private static byte[] randomBytes(int minSize, int maxSize, Random random)
+ final static Supplier<Gen<Integer>> leftPadding = () -> integers().between(0, 2);
+ final static Supplier<Gen<Integer>> rightPadding = () -> integers().between(0, 2);
+ final static Supplier<Gen<ValueAccessor>> accessors = () -> SourceDSL.arbitrary().pick(ACCESSORS);
+ final static Supplier<Gen<Function>> paddings = () -> leftPadding.get().zip(rightPadding.get(), (l, r) -> v -> maybePad(v, l, r));
+ final static byte[] bytes = new byte[200];
+
+ @BeforeClass
+ public static void beforeClass()
{
- int size = random.nextInt(maxSize - minSize + 1) + minSize;
- byte[] bytes = new byte[size];
- random.nextBytes(bytes);
- return bytes;
+ new Random(0).nextBytes(bytes);
+ }
+
+ /**
+ * This method should be used to test values to be processed by {@link ValueAccessor}
+ * in order to make sure we do not assume position == 0 in the underlying {@link ByteBuffer}
+ */
+ public static <V> V maybePad(V value, int lPad, int rPad)
+ {
+ if (value instanceof ByteBuffer)
+ {
+ ByteBuffer buf = ByteBuffer.allocate(((ByteBuffer) value).remaining() + lPad + rPad);
+ buf.position(lPad);
+ buf.duplicate().put((ByteBuffer) value);
Review comment:
The use of `duplicate` is confusing. Using `put` directly on the buffer and ressetting the position to `lpad` achieve the same result in a way easier to understand and without creating an unecessary object.
##########
File path: test/unit/org/apache/cassandra/db/marshal/ValueAccessorTest.java
##########
@@ -20,75 +20,95 @@
import java.io.IOException;
import java.nio.ByteBuffer;
-import java.util.Arrays;
import java.util.Random;
+import java.util.function.Function;
+import java.util.function.Supplier;
-import com.google.common.primitives.Shorts;
+import org.apache.commons.lang3.tuple.ImmutablePair;
+import org.apache.commons.lang3.tuple.ImmutableTriple;
import org.junit.Assert;
+import org.junit.BeforeClass;
import org.junit.Test;
import org.apache.cassandra.io.util.DataOutputBuffer;
-import org.apache.cassandra.utils.ByteArrayUtil;
import org.apache.cassandra.utils.ByteBufferUtil;
import org.assertj.core.api.Assertions;
import org.quicktheories.core.Gen;
import org.quicktheories.generators.SourceDSL;
import static org.apache.cassandra.db.marshal.ValueAccessors.ACCESSORS;
+import static org.apache.cassandra.utils.ByteArrayUtil.bytesToHex;
+import static org.apache.cassandra.utils.ByteBufferUtil.bytesToHex;
import static org.quicktheories.QuickTheory.qt;
+import static org.quicktheories.generators.SourceDSL.arbitrary;
+import static org.quicktheories.generators.SourceDSL.doubles;
+import static org.quicktheories.generators.SourceDSL.integers;
public class ValueAccessorTest
{
- private static byte[] randomBytes(int minSize, int maxSize, Random random)
+ final static Supplier<Gen<Integer>> leftPadding = () -> integers().between(0, 2);
+ final static Supplier<Gen<Integer>> rightPadding = () -> integers().between(0, 2);
+ final static Supplier<Gen<ValueAccessor>> accessors = () -> SourceDSL.arbitrary().pick(ACCESSORS);
+ final static Supplier<Gen<Function>> paddings = () -> leftPadding.get().zip(rightPadding.get(), (l, r) -> v -> maybePad(v, l, r));
+ final static byte[] bytes = new byte[200];
+
+ @BeforeClass
+ public static void beforeClass()
{
- int size = random.nextInt(maxSize - minSize + 1) + minSize;
- byte[] bytes = new byte[size];
- random.nextBytes(bytes);
- return bytes;
+ new Random(0).nextBytes(bytes);
+ }
+
+ /**
+ * This method should be used to test values to be processed by {@link ValueAccessor}
+ * in order to make sure we do not assume position == 0 in the underlying {@link ByteBuffer}
+ */
+ public static <V> V maybePad(V value, int lPad, int rPad)
+ {
+ if (value instanceof ByteBuffer)
+ {
+ ByteBuffer buf = ByteBuffer.allocate(((ByteBuffer) value).remaining() + lPad + rPad);
+ buf.position(lPad);
+ buf.duplicate().put((ByteBuffer) value);
+ buf.limit(buf.limit() - rPad);
+ return (V) buf;
+ }
+ else
+ {
+ return value;
+ }
}
- private static ValueAccessor<?> nextAccessor(Random random)
+ private static byte[] randomBytes(int size)
{
- return ACCESSORS[random.nextInt(ACCESSORS.length)];
+ assert size <= ValueAccessorTest.bytes.length;
+ byte[] bytes = new byte[size];
+ System.arraycopy(ValueAccessorTest.bytes, 0, bytes, 0, size);
+ return bytes;
}
- private static <V1, V2> void testHashCodeAndEquals(int seed, ValueAccessor<V1> accessor1, ValueAccessor<V2> accessor2, Random random)
+ private static <V1, V2> void testHashCodeAndEquals(int size, int sliceOffset, int sliceSize,
+ ValueAccessor<V1> accessor1, Function<V1, V1> padding1,
+ ValueAccessor<V2> accessor2, Function<V2, V2> padding2)
{
- byte[] rawBytes = randomBytes(2, 200, random);
- int sliceOffset = random.nextInt(rawBytes.length);
- int sliceSize = random.nextInt(rawBytes.length - sliceOffset);
- V1 value1 = accessor1.slice(accessor1.valueOf(rawBytes), sliceOffset, sliceSize);
- V2 value2 = accessor2.slice(accessor2.valueOf(rawBytes), sliceOffset, sliceSize);
+ byte[] rawBytes = randomBytes(size);
+ V1 value1 = accessor1.slice(padding1.apply(accessor1.valueOf(rawBytes)), sliceOffset, sliceSize);
+ V2 value2 = accessor2.slice(padding2.apply(accessor2.valueOf(rawBytes)), sliceOffset, sliceSize);
Review comment:
I do not believe that using `slice` here makes a lot of sense (I know that it was there before :-)). It looks like it was just some way to have some randomness in the array shape. We could simply use `valueOf` on an array generated by quicktheories. The `slice` method probably deserve a test in itself.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org