dcapwell commented on code in PR #50: URL: https://github.com/apache/cassandra-accord/pull/50#discussion_r1261808610
########## accord-core/src/test/java/accord/utils/SimpleBitSetTest.java: ########## @@ -0,0 +1,203 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package accord.utils; + +import java.util.ArrayList; +import java.util.BitSet; +import java.util.List; +import java.util.Random; +import java.util.function.BiConsumer; +import java.util.function.IntConsumer; + +import com.google.common.collect.Lists; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +public class SimpleBitSetTest +{ + private static final int NOT_FOUND = Integer.MAX_VALUE; + + private static class Check + { + final SimpleBitSet test; + final BitSet canon; + final int size; + + private Check(SimpleBitSet test, BitSet canon, int size) + { + this.test = test; + this.canon = canon; + this.size = size; + } + + void check(Random random) + { + assertEquals(canon.cardinality(), test.setBitCount()); + assertEquals(canon.nextSetBit(0), test.firstSetBit()); + assertEquals(normaliseNotFound(canon.nextSetBit(0)), test.firstSetBit(NOT_FOUND)); + assertEquals(canon.previousSetBit(size), test.lastSetBit()); + assertEquals(normaliseNotFound(canon.previousSetBit(size)), test.lastSetBit(NOT_FOUND)); + + forIndices(random, i -> assertEquals(canon.nextSetBit(i), test.nextSetBit(i))); + forIndices(random, i -> assertEquals(normaliseBefore(canon.nextSetBit(0), i), test.firstSetBitBefore(i))); + forIndices(random, i -> assertEquals(normaliseNotFoundBefore(canon.nextSetBit(0), i), test.firstSetBitBefore(i, NOT_FOUND))); + forRanges(random, (i, j) -> assertEquals(normaliseBefore(canon.nextSetBit(i), j), test.nextSetBitBefore(i, j))); + forRanges(random, (i, j) -> assertEquals(normaliseNotFoundBefore(canon.nextSetBit(i), j), test.nextSetBitBefore(i, j, NOT_FOUND))); + + forIndices(random, i -> assertEquals(i == 0 ? -1 : canon.previousSetBit(i - 1), test.prevSetBit(i))); + forIndices(random, i -> assertEquals(normaliseNotBefore(size == 0 ? -1 : canon.previousSetBit(size - 1), i), test.lastSetBitNotBefore(i))); + forIndices(random, i -> assertEquals(normaliseNotFoundNotBefore(size == 0 ? -1 : canon.previousSetBit(size - 1), i), test.lastSetBitNotBefore(i, NOT_FOUND))); + forRanges(random, (i, j) -> assertEquals(normaliseNotBefore(j == 0 ? -1 : canon.previousSetBit(j - 1), i), test.prevSetBitNotBefore(j, i))); + forRanges(random, (i, j) -> assertEquals(normaliseNotFoundNotBefore(j == 0 ? -1 : canon.previousSetBit(j - 1), i), test.prevSetBitNotBefore(j, i, NOT_FOUND))); + + List<Integer> canonCollect = new ArrayList<>(), testCollect = new ArrayList<>(); + canon.stream().forEachOrdered(canonCollect::add); + test.forEach(testCollect, List::add); + assertEquals(canonCollect, testCollect); + + canonCollect = Lists.reverse(canonCollect); + testCollect.clear(); + test.reverseForEach(testCollect, List::add); + assertEquals(canonCollect, testCollect); + } + + void forIndices(Random random, IntConsumer consumer) + { + for (int c = 0 ; c < 100 ; ++c) + { + int i = random.nextInt(size + 1); + consumer.accept(i); + } + } + + void forRanges(Random random, BiConsumer<Integer, Integer> consumer) + { + for (int c = 0 ; c < 100 ; ++c) + { + int i = random.nextInt(size + 1); + int j = random.nextInt(size + 1); + if (i > j) { int t = i; i = j; j = t; } + consumer.accept(i, j); + } + } + + static Check generate(Random random, int maxSize, int modCount, int runLength, float runChance, float clearChance) + { + int size = random.nextInt(maxSize); + runLength = Math.min(size, runLength); + BitSet canon = new BitSet(size); + SimpleBitSet test = new SimpleBitSet(size); + if (size > 0) + { + while (modCount-- > 0) + { + boolean set = random.nextFloat() >= clearChance; + boolean run = runLength > 1 && random.nextFloat() < runChance; + if (run) + { + int i = random.nextInt(size); + int j = random.nextInt(size); + if (j < i) { int t = i; i = j; j = t; } + j = Math.min(i + 1 + random.nextInt(runLength - 1), j); + + if (set) + { + canon.set(i, j); + test.setRange(i, j); + } + else + { + canon.clear(i, j); + while (i < j) + test.unset(i++); + } + } + else + { + int i = random.nextInt(size); + if (set) + { + assertEquals(!canon.get(i), test.set(i)); + canon.set(i); + } + else + { + assertEquals(canon.get(i), test.unset(i)); + canon.clear(i); + } + } + assertEquals(canon.cardinality(), test.setBitCount()); + } + } + return new Check(test, canon, size); + } + } + + @Test + public void testRandomBitSets() + { + Random random = new Random(); + long seed = random.nextLong(); + System.err.println("Seed: " + seed); + random.setSeed(seed); + testRandomBitSets(random, 100000); Review Comment: > What exactly is the issue you have found? logging a seed is problematic for a few reasons: 1) if the class has multiple tests, the logs may not line up properly in the JUnit XML. It is common to see 0 or multiple seeds in a single test class as the logs do not properly lineup with the correct method. You may also see a single seed, but it's for a different test case. 2) JUnit can and will truncate logs if there is too much, so when we log before the test we may be impacted by this truncation. 3) The class may finish before the logging flushes, which means the seed gets lost 4) Jenkins does not keep the log output and 100% relies on the JUnit XML file. Circle CI keeps it, so for classes with a single test you can rely on the log file rather than the XML file, but this now requires developers to find the log file for the failing test (you look at the test report, find the class, go to the archive, find the split it ran in, then find the test class log file, then download it); Circle CI is going away, so we will be moving forward with the Jenkins behavior and rely only on the JUnit XML... The common solution for this is to use a top level exception that includes the seed ``` try { // do work } catch (Throwable t) { throw new AssertionError("Error detected for seed " + seed, t); } ``` JUnit will see the exception, so we know this will always make it into the JUnit XML; we don't worry about logging. This is my main solution for each example I have given above... Now, once we fix it so we throw an exception, we can take the seed and replay it, but if you want to add a break point *before* the failing case, you need to add a break point at the point of code that throws an issue, then go backwards to find the loop, then find the loop variable (such as `I = 109584`, then create a new break point at the loop where you stop right before the test... this is annoying... in `qt` builder we have ability to denote that the test is *pure* or not, which means *pure* tests (such as this one) report the seed used for that single attempt, so when it gets replayed the failing test is the first iteration (rather than the 109584 in my example). This is a usability improvement and not a "correctness" issue. > a rather than an argument in favour of a favoured toolchain. you can also look at this as a code duplication thing, rather than having the same code over and over again, you just have a single function ``` fuzz().run(YourClass::doWork); ``` rather than having to do ``` long seed = yourSeedGeneratorHere(); RandomSource rand = new RandomSource(seed); int I = 0; try { for (; I < 10_000; I++) doWork(rand); } catch (Throwable t) { throw new AssertionError("Error for seed " + seed + " at iteration " + i, t); } ``` I don't personally see value in hand rolling as it's more verbose. Now, there are cases where it totally makes sense to roll your own (such as Simulator), but for a property style test, do we really need a new framework? NOTE: * I use `qt` function to make it easier to migrate off QuickTheories once accord lands in trunk... My goal is to remove that dependency and just change the imports to point to the accord version... we can name it w/e we want, but that is the history behind the name -- 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]
