-
-
Notifications
You must be signed in to change notification settings - Fork 810
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Add tests for script invocation #1337
Conversation
def test_compile_one_file(script_runner, tmp_vy_file_1): | ||
ret = script_runner.run('vyper', '-f', 'bytecode', tmp_vy_file_1) | ||
|
||
assert ret.stdout == '0x6100c256600035601c52740100000000000000000000000000000000000000006020526f7fffffffffffffffffffffffffffffff6040527fffffffffffffffffffffffffffffffff8000000000000000000000000000000060605274012a05f1fffffffffffffffffffffffffdabf41c006080527ffffffffffffffffffffffffed5fa0e000000000000000000000000000000000060a052630dbe671f60005114156100b85734156100ac57600080fd5b600160005260206000f3005b60006000fd5b6100046100c2036100046000396100046100c2036000f3\n' # noqa: E501 |
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.
@jacqueswww how fragile would it be to test against raw bytecode for these simple programs? It shouldn't have to be updated too often due to language semantics changes, right?
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.
Since these tests are against the raw compiled output I'm curious to explore ways to make them less fragile. It seems like lots of things could change the resulting bytecode output making these tests fragile (but still valuable). Here are some thoughts.
- Instead of testing the raw output, test the resulting contract functionality. Parse the output -> deploy the contract -> call some functions -> test the returned values.
- Property based testing.
- raw output should contain the
0x61...
preamble - raw output should contain the
PUSH <42-as-literal>
sequence of instructions. - raw output should be hex string of at least length XXX
- Hamming distance between expected output and actual output is within X, to allow for small changes to resulting bytecode.
tests/bin/test_vyper.py
Outdated
|
||
@pytest.fixture | ||
def make_tmp_vy_file(tmp_path): | ||
name_counter = 0 |
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.
Using an itertools.count
for this type of thing can be a bit cleaner in cases like this where you just need blind increments. I think it allows you to remove the need for nonlocal
and in theory you can inline the incrementing into the string formating via f'{tmp{next(name_counter)}.vy'
if you're feeling brave.
Couldn't having a more flexible test be just as brittle but in a way that's harder to detect? For example, imagine we're searching for the preamble and push op. Then, someone breaks the compiler by causing the push op to appear in the constructor instead of the method. We wouldn't know anything was wrong (actually, I guess we probably would because other tests would almost certainly fail but you can still see what I mean). On the other hand, if you are making a change that has that effect, you expect to see tests break. Looking for a literal value gives the highest likelihood that the tests will fail in an expected way. |
Yes, though I'd argue that these are integration tests or something like that, and like you said, we focus on catching functionality errors at a lower level, and design this suite to ensure the CLI compilation produces the expected output. I recognize the impreciseness of that definition and the somewhat circular definition. Only trying to brainstorm ways to get similar guarantees from the tests while reducing likelihood that they end up being brittle. |
Just bombing in to say this is PR leet congrats :-) P.S. will check this PR out tomorrow. |
@jacqueswww @pipermerriam Just as a reminder, I see this PR as being very far from complete! It was really just the first couple of steps. :) |
732aad7
to
4987156
Compare
@jacqueswww Okay, haha. I just looked at the PR number and now I get what you meant :). |
@davesque any updates on this one? |
@fubuloubu Not currently. I can potentially get back on this after doing the eth-abi strategies and juggle it with the AST class work. |
Awesome! Would be helpful I think, we've had a few strange issues since the script is largely untested. |
@pipermerriam I'm looking over this discussion some time later now and, in retrospect, I completely agree with the points you were making earlier. We could probably find ways to make things a bit more robust. |
What I did
Added a test suite to ensure proper functioning of bin scripts.
How I did it
Added
pytest-console-scripts
plugin to testing requirements and bumped requiredpytest
version to take advantage of built-in temp file fixtures. Added test suite for scripts.How to verify it
Run tests.
Cute Animal Picture