dcapwell commented on code in PR #57:
URL: https://github.com/apache/cassandra-accord/pull/57#discussion_r1302065749
##########
accord-core/src/test/java/accord/burn/random/FrequentLargeRange.java:
##########
@@ -18,59 +18,121 @@
package accord.burn.random;
-import accord.utils.Invariants;
+import accord.utils.Gen;
+import accord.utils.Gen.LongGen;
+import accord.utils.Gens;
import accord.utils.RandomSource;
-public class FrequentLargeRange implements RandomLong
+import java.time.Duration;
+import java.util.concurrent.TimeUnit;
+
+public class FrequentLargeRange implements LongGen
{
- private final RandomLong small, large;
- private final double ratio;
- private final int steps;
- private final double lower, upper;
- private int run = -1;
- private long smallCount = 0, largeCount = 0;
-
- public FrequentLargeRange(RandomLong small, RandomLong large, double ratio)
+ private final LongGen small, large;
+ private final Gen<Boolean> runs;
+
+ public FrequentLargeRange(LongGen small, LongGen large, double ratio)
{
- Invariants.checkArgument(ratio > 0 && ratio <= 1);
this.small = small;
this.large = large;
- this.ratio = ratio;
- this.steps = (int) (1 / ratio);
- this.lower = ratio * .8;
- this.upper = ratio * 1.2;
+ this.runs = Gens.bools().biasedRepeatingRuns(ratio);
Review Comment:
> Is there a reason some (e.g. pick) methods warrant entry to RandomSource
but not others?
some picks have state, some do not; some are light weight, some are not. I
have been moving the picks with state outside as they need some extra state and
a type to hold it. so `pick(list)` can stay in the source as its stateless,
but `pickWithWeight(list)` "should" stay outside as it needs to hold some state
(the weight). I actually don't mind moving `pick(list)` out into a static
function, I just saw it as a very common pattern so felt it worked best in the
interface in the current usage.
```
rs.pick(a, b, c)
// vs
pick(rs, a, b, c)
```
I am personally fine with both, but feel that the below is bad as it imposed
a cost at each invoke, so makes more sense to push outside as it "should"
return a holder of this state (Supplier, Gen, w/e).
```
rs.pickWithWeight(a, b, c)
```
feel free to call me out for my inconsistent stance with `pick(Set)`, this
one violates what I stated above, and prob shouldn't be there; I mostly put it
there out of laziness and to fix bugs I found with picking form sets in a
non-deterministic manner... atm the function is dead code as its only used in
my follow up work, so I am also cool with dropping it for now till its used.
> is why I created the abstraction
I am the author, I think you mean why you did similar in Simulator? When I
created this interface I wanted to make sure to have what you needed in
simulator, what I needed in property tests, and perfectly match the JDK
interface to make migration easier. When I created this interface, I wanted to
make sure it worked in all worlds and didn't impose anything on anyone...
> I think we're going to continue to have friction here. I will continue to
embellish RandomSource as this is the natural place for me and how I create
tests, and is why I created the abstraction. You will continue to put
generators into Gen because this is how you prefer to write tests, and why you
created that abstraction. This is fine while the approaches do not mix; we
already have plenty of redundancy in test authorship. But we either have to
accept that we have different approaches and stick to the approach already in
use when editing tests, or we have to standardise.
This feels like a better conversation for dev@ or slack with a wider group
of opinions; you prefer `Supplier`, I prefer `Gen`, but both work from a
technical point of view so we need a tie breaking to move forward IMO.
> I have generally tried to avoid imposing my views on your approach, as I
don't see any reason to debate abstractions I don't intend to use.
We can choose to live in different worlds, but to be frank I am not the one
imposing my views. In the eviction branch I pointed out your property tests
had issues with them and showed how to fix them but also mentioned you didn't
need to write as the code existed; I let you choose which way to go. In this
patch your published stance is that I must conform to your view else I am
blocked...
> But in this case we're editing my work done with my approach
how are we editing *your* work when I am the author of this class? This
argument holds in https://github.com/apache/cassandra-accord/pull/58 as you are
refactoring my work; this patch does not impose a `Gen` on *your* work; your
patch does impose your views on me though.
> We seem to have as a result slipped into assuming we need to come to a
conclusion about the one true approach, but I fear on this we will ever reach
agreement. If we can agree not to impose our approaches on the other, and when
editing tests to try to obey the existing style (as we do elsewhere in the
codebase), I do not see why both (and more!) approaches, and generators, cannot
co-exist.
I would prefer this, we even had a similar conversation in dev@ when SAI
wanted to add a new random testing library to the project; the stance we moved
forward with is allow all to live.
But to restate what I said above, I am not forcing my stuff on you, you are
forcing yours on me.
--
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]
