-
Notifications
You must be signed in to change notification settings - Fork 697
[WIP] [BLOCKED] Add Python 3.8 testing #1885
Changes from all commits
8120716
ead4e57
de0049f
9bf0940
3c65440
7497337
07b4398
851c141
97cc8db
7350ac3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -51,7 +51,7 @@ def __init__( | |||||
| self, | ||||||
| db: AtomicDatabaseAPI, | ||||||
| execution_context: ExecutionContextAPI, | ||||||
| state_root: bytes) -> None: | ||||||
| state_root: Hash32) -> None: | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @carver This is the example of cargo-culting I mentioned on gitter: consumer changed to fit the ABC; previous commit went the other way (ABC changed to appease the consumer).
... so I gave it exactly what it asked. But I don't know if it's right or not.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, in this case, it looks like the right thing to do to go with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should Lines 135 to 136 in 0d79bb6
This doesn't produce a linting error; just wondering if this would be right to change now, too. |
||||||
| self._db = db | ||||||
| self.execution_context = execution_context | ||||||
| self._account_db = self.get_account_db_class()(db, state_root) | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -203,7 +203,10 @@ def test_new_vs_reference_code_stream_iter(bytecode): | |
| assert latest.program_counter == reference.program_counter | ||
|
|
||
|
|
||
| @given(read_len=st.integers(min_value=0), bytecode=st.binary(max_size=128)) | ||
| @given( | ||
| read_len=st.integers(min_value=0, max_value=2048), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This (here and in test below) certainly requires review.
See also commit message, copied below. A failure happens because the underlying io.BytesIO.read(size) call gets a size=9223372036854775808. That's (2^63), but it seems that the biggest size that BytesIO can read is (2^63)-1. The error surfaced after an update of package Anyway, limiting read_len fixes the issue. Could use (2^63)-1, too, but considering that Likely a limitation of CPython; I'd guess the read index is an
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems fine to me. |
||
| bytecode=st.binary(max_size=128) | ||
| ) | ||
| def test_new_vs_reference_code_stream_read(read_len, bytecode): | ||
| reference = SlowCodeStream(bytecode) | ||
| latest = CodeStream(bytecode) | ||
|
|
@@ -217,7 +220,7 @@ def test_new_vs_reference_code_stream_read(read_len, bytecode): | |
|
|
||
| @given( | ||
| read_idx=st.integers(min_value=0, max_value=10), | ||
| read_len=st.integers(min_value=0), | ||
| read_len=st.integers(min_value=0, max_value=2048), | ||
| bytecode=st.binary(max_size=128), | ||
| ) | ||
| def test_new_vs_reference_code_stream_read_during_iter(read_idx, read_len, bytecode): | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
ABC made same as the consumer:
py-evm/eth/vm/opcode.py
Lines 39 to 43 in 3f33ac8
Before the change,
mypysaid: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.
Yeah, I agree with your call here, too. The implementation is clearly returning an
Opcodeinstance instead of class, and would require a lot of refactoring to change (if we even wanted to).The only thing I'd add it that the ABC docs should update, too.