Abandon decided blocks #2968
Abandon decided blocks #2968
Conversation
| // Poll = 0 terminates. This will accept blocks 0 and 1. This will also reject block 3. | ||
| // Block = 4 will attempt to be delivered, but because it is effectively rejected due to the acceptance of block 1, it will be dropped. | ||
| // Poll = 1 should terminate and block 2 should be repolled. | ||
| func TestEngineVoteStallRegression(t *testing.T) { |
There was a problem hiding this comment.
(No action required) Could this behavior be validated in a simpler fashion? While this test may accurately reproduce the failure scenario, it's complexity may complicate future maintenance and may not be the best model for future test additions.
There was a problem hiding this comment.
This is basically a "black box" test of the engine... I wanted to avoid touching the internals (to avoid future maintenance burden).
We could additionally add a "clear box" test of deliver as a very targeted test for this behavior. Would that make sense?
There was a problem hiding this comment.
This is basically a "black box" test of the engine... I wanted to avoid touching the internals (to avoid future maintenance burden).
Fair enough, always a balancing act.
We could additionally add a "clear box" test of
deliveras a very targeted test for this behavior. Would that make sense?
Ideally every method gets as much coverage as makes sense, but I'm not sure how valuable a whitebox test would be without a refactor due to how dependent it is on state other than the method inputs.
There was a problem hiding this comment.
Yeah... The engine code is probably always going to have this job queue logic (So there will be some state between calls)... But I do hope that we will be able to significantly improve the testability + readability of the engine in some future PRs.
What I really want this to look like at the end of the day is specifying the "initial" state of the engine (with pending jobs and pending polls) and then verify the "output" state of the engine after some event happens... But that's really a large refactor (which is ongoing, but not for this PR)
Why this should be merged
This ensures that there will never be any jobs blocking on blocks that failed issuance due to already being decided.
How this works
How this was tested