-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backingstore-agnostic property tests using model-based testing #4081
Backingstore-agnostic property tests using model-based testing #4081
Conversation
d1f7807
to
1fd7c60
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding this test 🙏 Looks great, I left some comments and to merge this I think we should:
- Address the FIXMEs.
- Perform coverage testing to make sure we're covering important test cases.
ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/HD/LMDB.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore.hs
Outdated
Show resolved
Hide resolved
24a9462
to
0560d73
Compare
aeca7b7
to
0adb51d
Compare
The PR is ready for a second review round. See the update Description of this PR for some info on the major changes that this PR makes.
|
4758336
to
9bda2bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should decide which of the remaining TODO
s in the code we should address.
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore.hs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks good, I do have some comments here and there
ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/OnDisk.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/HD/BackingStore.hs
Show resolved
Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/HD/LMDB/PersistentTransaction.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore.hs
Show resolved
Hide resolved
...oros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore/Lockstep.hs
Outdated
Show resolved
Hide resolved
...oros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore/Lockstep.hs
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore/Mock.hs
Show resolved
Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/HD/LMDB/Bridge.hs
Show resolved
Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Storage/LedgerDB/HD/LMDB/PersistentTransaction.hs
Outdated
Show resolved
Hide resolved
...oros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore/Lockstep.hs
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore.hs
Show resolved
Hide resolved
ouroboros-consensus-test/test-storage/Test/Ouroboros/Storage/LedgerDB/HD/BackingStore/Mock.hs
Show resolved
Hide resolved
1f1ab01
to
2666381
Compare
There's no 0.2.1 version of quickcheck-dynamic that's going to be released, the next version will be 3.0.0 because it's a breaking change from 2.0.0 which is the latest published on hackage. |
Also, I am not too sure what |
2666381
to
4a439ca
Compare
* Added an `LMDB.Bridge` module that "bridges the gap" between the `Serialise`-based interface of `lmdb-simple`, and the `CodecMK`-based interface that we use throughout consensus. I moved some of the relevant definitions from the `LMDB` module to this `LMDB.Bridge` module. * Added an `LMDB.PersistentTransaction` module that keeps an internal LMDB transaction handle open, such that we can submit regular transactions to the same consistent view. This replaces the `ValueHandle` that was originally in the `LMDB` module. * Refactored range reads to use the new cursor API. This solves the bug where range reads previously were returning incorrect results. * Ensured that the LMDB backing store behaves according to the backing store specification in case the backing store or its value handles have been closed.
Ensured that the InMemory backing store behaves according to the backing store specification in case the backing store or any of its handles has been closed.
* Generalise `ioHasFS` to `MonadIO`. * Give a general `Show` instance `ApplyMapKind'`.
About the new modules: * `Registry.hs` defines a small utility for handling a resource through handles. This simplifies the property tests we define in other modules. * `Mock.hs` defines a mocked version of a `BackingStore` that generalises over the types keys, values and diffs we want to use. * `LockStep.hs` instantiates the Lockstep framework. * `BackingStore.hs` sets up and runs the Lockstep tests. Other changes: * Added a utility module that defines a simple `LedgerState` and corresponding `TableStuff` instance.
4a439ca
to
7749863
Compare
Thank you for pointing this out, I've amended this in the code and the corresponding GH issue |
7749863
to
f5577ad
Compare
I don't really understand this sentiment. Sure, of course, technically speaking it's true that |
@edsko I was expressing my sentiment in the context of those tests, not in general. I failed to see the added value in this particular case because it seemed to me the checks could have been equally well, and perhaps more simply (ie. without an additional conceptual layer added), expressed with qc-d. I may be wrong of course. |
…-property-tests Backingstore-agnostic property tests using model-based testing
Description
Resolves #3954.
Previously, the
BackingStore
functionality was only being tested through higher-level tests like theOnDisk
tests. This PR introduces QSM property tests that exercise theBackingStore
API directly. The tests can be reused for any type of backing store that implements theBackingStore
API. As a result of these tests a bug was identified in the implementation of range reads for the LMDB backing store. This PR also fixes this bug by re-implementing the range reads using a new LMDB cursor monad that is introduced by a PR in thelmdb-simple
package: input-output-hk/lmdb-simple#1Update 2022-12-05
Since the last review round, the tests have been refactored to use
quickcheck-lockstep
instead of QSM. Furthermore, the LMDB and InMem backing store have seen some refactoring as well. Changes include:LMDB.Bridge
module.Transaction
type from the LMDB bindings that keeps open a transaction handle for as long as we need it to.Checklist
interface-CHANGELOG.md
interface-CHANGELOG.md