Tuple Index Into Minor Fix #663
Conversation
|
I propose an additional test for |
| @@ -25,7 +25,7 @@ int 5 | |||
| extract_uint64 | |||
| store 2 | |||
| load 19 | |||
| extract 13 0 | |||
| extract 13 32 | |||
There was a problem hiding this comment.
So was the integration test giving a false negative?
There was a problem hiding this comment.
I don't think integration test (in general) is false negative, but the testcase is really a corner case we didn't account for.
There was a problem hiding this comment.
I think this is extracting the address which is at the end of the encoding, end therefore extract 13 0 extract 13 32. Does that make sense given your changes?
There was a problem hiding this comment.
Okay, this two cases are equal only under a precondition, the last element in tuple (address in this case) has no leading dynamic type(s).
If we both agree on this precondition, then yes, it makes sense given my change.
yes, working on it |
| @@ -371,6 +371,15 @@ class IndexTest(NamedTuple): | |||
| encoded, start_index=pt.ExtractUint16(encoded, pt.Int(0)) | |||
| ), | |||
| ), | |||
| IndexTest( | |||
There was a problem hiding this comment.
I tried this test case against master (and also the graviton roundtrip test that I proposed) and both tests passed. I'd like to get a better understanding of why that is. For the graviton roundtrip test, I guess the test is passing because it's decoding the entire tuple as opposed to a portion of it.
There was a problem hiding this comment.
good point on reverse testing on master, I am also trying to replicate things on master and will get back to you what I have.
There was a problem hiding this comment.
ok, my first find: this is INDEED an insufficient test: for we are taking from byte, rather than a static array (with static type). So the compilation will use getbyte, which makes no difference with or without length.
I propose we should introduce another type spec (like static bytes) for this testcase.
There was a problem hiding this comment.
made a change in 7bc748d, and it should be failing on master
ahangsu
force-pushed
the
tuple-index-into-fix
branch
from
45d6e31
to
7bc748d
Compare
No description provided.