Skip to content
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

Refactor bytecode comparison scripts #10675

Merged
merged 7 commits into from
Jan 11, 2021

Conversation

cameel
Copy link
Member

@cameel cameel commented Dec 22, 2020

Fixes #10165.

This is a refactor of the prepare_report.py script used for generating a big bytecode+metadata dump from a solc executable. For #10183 I need to expand the script a bit to make it backwards-compatible with old binaries, which requires making it work without --standard-json (#4698) and a few other workarounds. The current structure was very minimalistic and also inconsistent with the equivalent JS snippet for soljson.js (formerly embedded in storebytecode.sh; I extracted it into prepare_report.js). Here I'm only fixing the inconsistencies, making the script more modular and also updating it to use modern features of Python (type annotations, data classes, inline string formatting) and its standard library (argument parser, pathlib, unit testing).

The PR also includes a simple setup for testing stuff from scripts/ (at least the Python stuff). I'm including tests for prepare_report.py and in the future we could add more for the rest. Ideally, our scripts would be so simple that they would not require separate testing but unfortunately that's not the case. We have some Bash stuff that could really be better off written in Python and tested.

I'm currently running tests manually with:

PYTHONPATH="scripts/:$PYTHONPATH" python -m unittest discover --start-directory test/scripts/

Later, probably in a separate PR, I'm going to hook it up in the CI so that it can run automatically.

@cameel cameel self-assigned this Dec 22, 2020
@cameel cameel force-pushed the refactor-bytecode-comparison-scripts branch 3 times, most recently from bbc7c26 to 170e193 Compare December 22, 2020 09:13
@cameel cameel force-pushed the refactor-bytecode-comparison-scripts branch from 170e193 to efe25ba Compare December 22, 2020 09:26
@cameel cameel force-pushed the refactor-bytecode-comparison-scripts branch from efe25ba to 085f88d Compare January 11, 2021 16:08
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks good to me, but the tests depend on the compiler version string and the produced bytecode - won't that be annoying because it'll break every version and especially on every bytecode-altering compiler change?
@cameel just mentioned that the tests use hardcoded version and bytecode, I just didn't look closely enough, so nevermind.

Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't really check the test cases added in the last commit, but the reports itself look good, so I'd trust it's fine.
The rest looks nice!

@chriseth chriseth merged commit d43693e into develop Jan 11, 2021
@chriseth chriseth deleted the refactor-bytecode-comparison-scripts branch January 11, 2021 17:57
@cameel
Copy link
Member Author

cameel commented Jan 12, 2021

@ekpyron

@cameel just mentioned that the tests use hardcoded version and bytecode, I just didn't look closely enough, so nevermind.

Yeah, tests added here never actually run solc. That wouldn't even work because I need this script to work with older compiler versions (upcoming PRs deal with that) and they're not available during test. And the bytecode tasks in CI will test it with solc anyway so the tests here only check specific functions with specific hard-coded input.

And these tests don't really cover everything, just the most important parts. I'm assuming we don't want to go overboard with testing scripts, especially when it requires writing Python code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up the bytecode comparison scripts
3 participants