[darcs-devel] TreeWithFlattenPos

Ben Franksen ben.franksen at online.de
Fri Jun 21 11:50:19 UTC 2019


See harness/Darcs/Test/Patch/Arbitrary/Generic.hs

It associates a number with a Tree. The Arbitrary instance tells me that
this number is supposed to be no greater than the number of pairs in any
flattened version of the Tree, i.e.  (max 0 (numberOfPatches t - 1)).

While commutePairFromTree always chooses the last of all pairs,
commutePairFromTWFP chooses an arbitrary pair by indexing into the list
of pairs with the number associated with the Tree. Why not use the oneof
combinator from QuickCheck and dispense with the number?

We test many properties with generators for Tree and for
TreeWithFlattenPos. Makes no sense to me at all. If the idea was to test
the pairs at the leaves of the tree more often, why not use frequency
for that? (But I could see no reason why we would want to do that).

BTW, numberOfPatches is almost the same as sizeTree, except that
sizeTree adds 1 for each Par Node, no idea why. Since sizeTree isn't
used anywhere except for propFail which in turn isn't used anywhere I
think it could safely be changed to return the actual size.

Also, I have the impression that the author of this module didn't
realize that all tree flattenings necessarily have the same size which
is exactly the number of patches in the Tree.

Unless someone chimes in here and comes up with a good explanation for
this apparently unnecessary complication I will go ahead and simplify
things by removing TreeWithFlattenPos and modifying the test generators
for pairs (and triples) such that they pick an arbitrary pair (or
triple) from any of the tree flattenings. I'll do this before I start
adding the missing scenarios (i.e. patches recorded on top of conflictors).

Cheers
Ben



More information about the darcs-devel mailing list