Add generalized property tests for coin selection algorithms. #81
Conversation
Ignore: * non-camel-case property function names. * nested lambda expressions. Both of these are actually rather useful in test code.
jonathanknowles
changed the title
| -- A collection of general properties that should apply to all algorithms | ||
| -- that implement the 'CoinSelectionAlgorithm' interface. | ||
| -- | ||
| coinSelectionAlgorithmGeneralProperties |
There was a problem hiding this comment.
why not to use Common or Generic ? it sounds better to me (I know you know better as you are native speaker :-) )
There was a problem hiding this comment.
why not to use Common or Generic ? it sounds better to me (I know you know better as you are native speaker :-) )
Good question!
I'm trying to capture a contrast between specialized and generalized.
We already have tests for properties that are specific (or specialized) to particular algorithms. For example, for Largest-First, we have a specialized property test that checks that all selected inputs are greater than or equal (in coin value) to the remaining inputs not selected.
However, the properties in this PR are all general (non-specific), as they apply to all implementations.
We could also use the term "common" here. I tried to avoid the word "generic" as it could be confused with the concept of generic programming.
| intersection :: Ord k => CoinMap k -> CoinMap k -> CoinMap k | ||
| intersection (CoinMap a) (CoinMap b) = CoinMap $ a `Map.intersection` b | ||
|
|
||
| union :: Ord k => CoinMap k -> CoinMap k -> CoinMap k |
There was a problem hiding this comment.
is this really needed when we have https://github.com/input-output-hk/cardano-coin-selection/blob/master/src/library/Cardano/CoinSelection.hs#L123 ?
There was a problem hiding this comment.
ah, ok. you wanted to make it explicit
There was a problem hiding this comment.
is this really needed when we have https://github.com/input-output-hk/cardano-coin-selection/blob/master/src/library/Cardano/CoinSelection.hs#L123 ?
Strictly speaking it's not needed!
Here my motivation was to make the tests more readable. Defining union (arguably) makes the test more readable at the call site.
In particular, the following two lines are now directly comparable to one another:
(selected `intersection` remaining) `shouldBe` mempty
(selected `union` remaining) `shouldBe` availableWhereas if we just use <> instead of union, it's no longer immediately clear that we're invoking a union operation (IMO).
jonathanknowles
changed the title
jonathanknowles
changed the title
Related Issue
#21
Summary
This PR adds missing tests for properties that apply to all algorithms implementing the
CoinSelectionAlgorithminterface, as defined by the coin selection specification:value inputsAvailable ≥ value outputsRequestedoutputsSelected = outputsRequestedinputsSelected ⊆ inputsAvailableinputsRemaining ⊆ inputsAvailableinputsSelected ⋂ inputsRemaining = ∅inputsSelected ⋃ inputsRemaining = inputsAvailablevalue inputsSelected = value outputsSelected + value changeThis PR validates the above set of properties against the following algorithms: