Generalize test machinery's reliance on TestBlock #21
Conversation
isovector
force-pushed
the
isovector/more-detest
branch
from
ac2dc3b to
65a4b82
Compare
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PeerSimulator/Config.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PointSchedule.hs
Outdated
Show resolved
Hide resolved
isovector
force-pushed
the
isovector/more-detest
branch
2 times, most recently
from
8dbfbed to
a89a58b
Compare
ninioArtillero
left a comment
ninioArtillero
left a comment
There was a problem hiding this comment.
LGTM! Thanks for this piece 👌
Just left a question.
| prop_chainSyncKillsBlockFetch :: Property | ||
| prop_chainSyncKillsBlockFetch = do | ||
| forAllGenesisTest | ||
| forAllGenesisTest @TestBlock |
There was a problem hiding this comment.
Why do we need the TestBlock type application here (and in the other properties/calls to forAllGenesistest)? I see that ghc does complain about ambiguity, but I don't understand why that is; perhaps this is pointing to future work to run this tests with cardano blocks? Shouldn't the parametrization introduced by the PR be enough for this to be resolved at runtime?
There was a problem hiding this comment.
Turns out there's nothing anywhere in this test that pins down the block type. Which is a pretty good thing---it means this test doesn't actually depend on TestBlock.
ouroboros-consensus-diffusion/test/consensus-test/Test/Consensus/PeerSimulator/Trace.hs
Outdated
Show resolved
Hide resolved
nbloomf
left a comment
nbloomf
left a comment
There was a problem hiding this comment.
This looks good to me. Do we know anything about the handful of tests that are currently failing? Does that situation predate this project or did we break something?
# Description This PR fixes `hashForkNo` as previous behavior uniquely identified branches by fork number and the new parameterized implementation did not. This seems relevant because otherwise `BlockTreeBranch`es forking from the same block in the trunk would end up with the same fork number. As it turns out, these _fork numbers_ are not related to the order in which forking takes place. Related documentation was added. This seem to be related to an apparent mismatch. On the one hand, the `Test.Util.TestBlock.TestHash` documentation states that multiple branches can steem from a single node in a tree and _suggests_ that the fork number indexes branches in an orderly way. On the other hand, the implementation of `GenesisTest` generation in `Test.Consensus.Genesis.Setup.GenChains` assigns fork numbers to branches in an arbitrary way.
isovector
force-pushed
the
isovector/more-detest
branch
from
a2a6456 to
0668a9d
Compare
Description
This PR generalizes all uses of unnecessarily-specified
TestBlocks to instead be parametricblks.