Enable and fix remaining transaction tests#34
Conversation
pipermerriam
left a comment
pipermerriam
left a comment
There was a problem hiding this comment.
Can you investigate and check to be sure this is all passing because of the right reasons.
| try: | ||
| normalized_fixture['rlp'] = decode_hex(fixture['rlp']) | ||
| except binascii.Error: | ||
| normalized_fixture['rlpHex'] = fixture['rlp'] |
There was a problem hiding this comment.
So this is actually intentional (and likely important) so that there is a clear and unambiguous differentiation between RLP that has been decoded to bytes from it's hex form, and RLP that failed decoding.
There was a problem hiding this comment.
@pipermerriam I changed this back and am now checking for for the KeyError when the tests try to call fixture['rlp'] (when it's now fixture['rlpHex'])
| try: | ||
| transaction = rlp.decode(fixture['rlp'], sedes=TransactionClass) | ||
| except rlp.exceptions.ObjectDeserializationError: | ||
| except (rlp.exceptions.ObjectDeserializationError, rlp.exceptions.DecodingError, TypeError): |
There was a problem hiding this comment.
I'm a little worried that the tests passing are because this is just catching all of the errors. I'm traveling today and don't have time to check this, but can you verify that the errors being caught here are appropriate error conditions by potentially logging out how many of the tests succeed because of the subsequent return statement.
There was a problem hiding this comment.
@pipermerriam I've attached the tests that are passing due to these exceptions, most of them have WrongRLP in the title and I believe the ones that don't should be caught here. I've attached a list of the errors that are passing due to the aforementioned exceptions.
passing_due_to_rlp_exception.txt
DavidKnott
force-pushed
the
enable-and-fix-transacion-tests
branch
from
b69f26c to
a035e67
Compare
| assert fixture['rlpHex'] | ||
| return | ||
| except TypeError as err: | ||
| assert err.args == ("'bytes' object cannot be interpreted as an integer",) |
There was a problem hiding this comment.
This is curious to me? What is causing this error?
There was a problem hiding this comment.
In fact, can you toss a comment into each of these except blocks with a brief explanation of what they are catching.
There was a problem hiding this comment.
Definitely, TypeError occurs on 4 tests all called RLPElementIsListWhenItShouldntBe and it occurs when it tries to change a list of bytes into an int
value = [b'dog', b'god', b'cat']
def big_endian_to_int(value):
return int.from_bytes(value, byteorder='big')
E TypeError: 'bytes' object cannot be interpreted as an integer
DavidKnott
force-pushed
the
enable-and-fix-transacion-tests
branch
from
a035e67 to
f868a6e
Compare
* bump versions in dependencies and ci builds * move tox to [dev] per issue ethereum#34 * move RTD deps pointer into .readthedocs.yml * unpin flake8 add flake8-bugbear to lint deps
* bump versions in dependencies and ci builds * move tox to [dev] per issue ethereum#34 * move RTD deps pointer into .readthedocs.yml * unpin flake8 add flake8-bugbear to lint deps
2 participants
This PR is in response to #32
It enables and fixes the remaining json-fixture transaction tests
Cute Animal Picture
http://www.cutestpaw.com/wp-content/uploads/2012/01/baby-tiger.jpg