Test that consensus emits valid CBOR #479
Conversation
ouroboros-consensus/src/unstable-consensus-testlib/Test/Util/Serialisation/Roundtrip.hs
Outdated
Show resolved
Hide resolved
dnadales
force-pushed
the
dnadales/3099-test-that-consensus-emits-valid-cbor
branch
from
73dcec6 to
24060a6
Compare
| @@ -61,7 +61,7 @@ tests = adjustOption reduceTests $ | |||
| , testGroup "SerialiseNodeToNode" $ | |||
| roundtrip_SerialiseNodeToNode byronToCardanoCodeConfig | |||
| , testGroup "SerialiseNodeToClient" $ | |||
| roundtrip_SerialiseNodeToClient byronToCardanoCodeConfig | |||
| roundtrip_SerialiseNodeToClient (const CheckCBORValidity) byronToCardanoCodeConfig | |||
There was a problem hiding this comment.
I don't think this is needed anymore.
There was a problem hiding this comment.
It turns out we do, due to IntersectMBO/cardano-ledger#3800
There was a problem hiding this comment.
I think keeping this is fine, we can remove the PParamsLegacyEncoder stuff after the next HF anyway.
dnadales
force-pushed
the
dnadales/3099-test-that-consensus-emits-valid-cbor
branch
2 times, most recently
from
add6a91 to
643e2fe
Compare
| -- | All roundtrip tests, skipping the specified CBOR validity tests. | ||
| -- | ||
| -- | ||
| -- TODO: the exclusion rule should only be considered for blocks __before__ Conway! |
There was a problem hiding this comment.
Would it make sense to address this as a separate issue?
There was a problem hiding this comment.
AFAICT, no Conway blocks are excluded at all, is this comment stale (ie a leftover from the FlatTerm roundtripping)?
There was a problem hiding this comment.
Oh, what I meant is that we are using this to skip checks for legacy encoders before Conway. However, we should have a way to enable roundtrip_SerialiseNodeToClient at and after Conway. So I was thinking about changing the type of the filter function to something like:
(a, TestName) -> ShouldCheckCBORValidity
| @@ -61,7 +61,7 @@ tests = adjustOption reduceTests $ | |||
| , testGroup "SerialiseNodeToNode" $ | |||
| roundtrip_SerialiseNodeToNode byronToCardanoCodeConfig | |||
| , testGroup "SerialiseNodeToClient" $ | |||
| roundtrip_SerialiseNodeToClient byronToCardanoCodeConfig | |||
| roundtrip_SerialiseNodeToClient (const CheckCBORValidity) byronToCardanoCodeConfig | |||
There was a problem hiding this comment.
I think keeping this is fine, we can remove the PParamsLegacyEncoder stuff after the next HF anyway.
| -- | All roundtrip tests, skipping the specified CBOR validity tests. | ||
| -- | ||
| -- | ||
| -- TODO: the exclusion rule should only be considered for blocks __before__ Conway! |
There was a problem hiding this comment.
AFAICT, no Conway blocks are excluded at all, is this comment stale (ie a leftover from the FlatTerm roundtripping)?
From 20% to 100%. We've seen that when nothing changes, benchmarks can be up to 40% slower on CI. So the threshold was bumped to account for this variability.
This commits also add a filtering mechanism to skip CBOR validity tests of legacy encoders.
dnadales
force-pushed
the
dnadales/3099-test-that-consensus-emits-valid-cbor
branch
from
0ddf493 to
55f49e6
Compare
dnadales
deleted the
dnadales/3099-test-that-consensus-emits-valid-cbor
branch
Fixes IntersectMBO/ouroboros-network#3099
Sadly some CBOR validity tests need to be skipped due to:
This patch introduces an ad-hoc filtering mechanism to fix problems with legacy encoders.