Binary Agreement test updated to the proptest framework #336
Conversation
|
|
||
| mod network; | ||
| pub mod net; |
There was a problem hiding this comment.
There are lots of "method is never used" lints otherwise in the net module.
There was a problem hiding this comment.
Right; some of them are already marked as #[allow(unused)]—maybe we should apply that attribute to the whole net module instead (either inside it, or at the use statements)? That would make the reason clearer than the pub.
There was a problem hiding this comment.
One option would also be to make the testing framework a proper crate and just import that; I've done this on another project.
It's a bit more work, but it also does not require importing a million different crates on each test cases - right now, whenever I add a dependency to the net testing framework, I also need to add one in every test case.
There was a problem hiding this comment.
You're right, I guess that would be the proper solution. 👍
(Something for another PR, though.)
vkomenda
force-pushed
the
vk-agreement-test
branch
from
4b71326
to
d912e33
Compare
|
@mbr, please check the usage of |
vkomenda
force-pushed
the
vk-agreement-test
branch
from
d912e33
to
5960ec1
Compare
vkomenda
force-pushed
the
vk-agreement-test
branch
from
5960ec1
to
9c207e7
Compare
There was a problem hiding this comment.
There are some things I would probably think about a bit further, however, we will have to re-examine this anyway once we get the nomenclature changes, fault log (#338) checking, the contested on-VirtualNet-observers and other things in.
The current version seems to run and is not a dependency of anything, so I am fine with moving along, merging this and then revisiting it later.
| /// Strategy to generate a test configuration. | ||
| fn arb_config() | ||
| ( | ||
| dimension in NetworkDimension::range(1, 50), |
There was a problem hiding this comment.
Just nothing that we need to be doubly sure 1 is a workable network size for the test (it's not for net_dynamic_hb, for example, since a node gets removed). Should you need to change it, beware that the 1-50 range here refers to the random initial size, not the minimum.
There was a problem hiding this comment.
I'd say we definitely want BA to work with a size of 1.
There was a problem hiding this comment.
If proptest reduces it to 1 in case of failure anyway, maybe it's worth starting from above 1.
In the process of updating the BA test I had to remove the observer check temporarily from that test until we implement observers in the test framework. See #322.
I also removed logging from the test in advance of
EventLog, see #335.