eip7805: add unit tests for InclusionListStore#4644
eip7805: add unit tests for InclusionListStore#4644jtraglia merged 4 commits intoethereum:masterfrom
InclusionListStore#4644Conversation
tests/core/pyspec/eth2spec/test/eip7805/unittests/inclusion_list/test_inclusion_list_store.py
Show resolved
Hide resolved
tests/core/pyspec/eth2spec/test/eip7805/inclusion_list/test_inclusion_list_store.py
Outdated
Show resolved
Hide resolved
| def test_inclusion_list_store_transaction_uniqueness(spec, state): | ||
| def run_func(): | ||
| forkchoice_store = get_genesis_forkchoice_store(spec, state) | ||
| inclusion_list_store = spec.get_inclusion_list_store() | ||
| inclusion_list_committee = spec.get_inclusion_list_committee(state, state.slot) | ||
|
|
||
| signed_inclusion_list_1 = get_sample_signed_inclusion_list( | ||
| spec, state, validator_index=inclusion_list_committee[0] | ||
| ) | ||
| signed_inclusion_list_2 = get_sample_signed_inclusion_list( | ||
| spec, state, validator_index=inclusion_list_committee[1] | ||
| ) | ||
| signed_inclusion_list_3 = get_sample_signed_inclusion_list( | ||
| spec, state, validator_index=inclusion_list_committee[2] | ||
| ) |
There was a problem hiding this comment.
Will these ILs share one or more transactions? It doesn't appear that they do. Can we add an assert for that somewhere?
There was a problem hiding this comment.
I've added two ILs with the same transactions. Let me know if we need an assertion as well!
There was a problem hiding this comment.
Not sure if you're planning on adding more, but it might be useful to add tests with:
- Inclusion lists with no transactions.
- Inclusion lists with the max number of transactions.
There was a problem hiding this comment.
I've added them in test_inclusion_list_store_transaction_uniqueness test. Let me know if we want to add them in other tests as well!
|
Thanks for feedback! Let me add more tests and the checklist in this PR. |
| """ | ||
|
|
||
| @lru_cache(maxsize=1) | ||
| def cached_or_new_inclusion_list_store(): |
There was a problem hiding this comment.
Are we adding this for speed of the tests, or are we testing the effect of caching on the protocol?
There was a problem hiding this comment.
It's one of ways to return a cached IL store or an empty IL store, if it's not exist. Another way is using global keyword. No strong preferences, we can change it to something else if the intention is not straightforward.
There was a problem hiding this comment.
My question is not about how you are caching it, but why are you replacing cached_or_new_inclusion_list_store, what are you trying to test with this?
There was a problem hiding this comment.
Like this seems to be the same as the default spec but just adding the caching.
There was a problem hiding this comment.
As the comment explains, it's monkeypatching it to make it viable. cached_or_new_inclusion_list_store itself is an implementation dependent function so you need to provide the impl somehow.
There was a problem hiding this comment.
These tests only call, get_inclusion_list_store once per test, so the cached value is never used. So functional it is the same as the default implementation.
Is there a reason why this is necessary for these tests, or is it for future tests?
There was a problem hiding this comment.
Now I see what you mean. I think having this monkeypatching function would be useful in other tests such as test_process_execution_payload. It allows us to set IL transactions in the IL store directly, which will make testing concise and straightforward. Furthermore, get_inclusion_list_store is called multiple times per test. on_inclusion_list calls get_inclusion_list_store internally.
Having other tests that set the content of IL store would've made it clearer to see if this could be useful. My half-finished test_process_execution_payload is in my stash but I've been working on other stuff, sorry about that. Intuitively, I found this symmetrical to retrieve_blobs_and_proofs or retrieve_column_sidecars.
There was a problem hiding this comment.
What do you think it we wait to include the test_process_execution_payload in this PR? That will make use of this, right?
I feel we should have a better way of setting the implementation of these. Perhaps it would be more pythonic doing it with wtih clause. But that would be a matter for another PR.
There was a problem hiding this comment.
All IL related tests would use this. test_process_execution_payload is a big change so I don't want to hold this PR that longer tbh. It will add a new test case format for ILs as well as modify run_with_inclusion_list_store signature.
Making these kinds of test more pythonic sounds good but I agree that it deserves another PR.
Could you elaborate your concern? I'm happy to discuss how we can improve this PR.
There was a problem hiding this comment.
Hey @leolara the tests will fail without the caching decorator. I tried a few other things too and it seems that the current implementation is necessary. I'm going to merge this PR. Please submit another PR with the changes you want to see and we can discuss it more there. That way we have a concrete proposal to see.
jihoonsong
force-pushed
the
inclusion-list-store-tests
branch
from
74c2550 to
ef6064b
Compare
3 participants
This PR adds unit tests for
InclusionListStore.