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

Acid tests should check bytecode too #52

Closed
ionelmc opened this issue Nov 20, 2012 · 5 comments
Closed

Acid tests should check bytecode too #52

ionelmc opened this issue Nov 20, 2012 · 5 comments

Comments

@ionelmc
Copy link

ionelmc commented Nov 20, 2012

The tests should verify that the pep8ized file generates the same bytecode as the original file.

There should be some sort of smart comparison because line numbers should not be compared.

I think is very important to verify that this tool doesn't actually change the code.

"Correct some non-idiomatic Python code." is mentioned in the readme but no details on what corrections are made ? This should be disabled for the bytecode match tests if it does code reductions or optimizations.

@myint
Copy link
Collaborator

myint commented Nov 20, 2012

I explored bytecode comparison when I first wrote the acid tests. I gave up on it once I found that things like has_key() and in didn't collapse into the same bytecode. But since autopep8 is now doing things a bit more complicated like shortening lines, it might be worth doing what you suggest. I suppose we would need to disable everything except whitespace, indentation, and line length changes.

By the way, the idiomatic changes I mention are basically the things 2to3 -f idioms fixes.

@myint
Copy link
Collaborator

myint commented Nov 22, 2012

See 8631ecb.

@myint
Copy link
Collaborator

myint commented Nov 22, 2012

I should probably do a recursive comparison of the code object rather than using dis. My current implementation with dis is basically just checking the top-level bytecode.

myint pushed a commit that referenced this issue Nov 22, 2012
Previously we were only disassembling the top-level code. Now we
recursively disassemble the lower levels. This resolves issue #52.
@myint myint closed this as completed Nov 22, 2012
@myint
Copy link
Collaborator

myint commented Nov 24, 2012

For the most part, this works well. But sometimes the same code section generates different, but equivalent, bytecode representations. For example

16 LOAD_CONST             338 ((('catalogue', '0001_initial'), ('customer', '0001_initial'), ('partner', '0001_initial'), ('address', '0001_initial')))

versus

16 LOAD_CONST               1 ('catalogue')
19 LOAD_CONST               2 ('0001_initial')
22 BUILD_TUPLE              2
25 LOAD_CONST               3 ('customer')
28 LOAD_CONST               2 ('0001_initial')
31 BUILD_TUPLE              2
34 LOAD_CONST               4 ('partner')
37 LOAD_CONST               2 ('0001_initial')
40 BUILD_TUPLE              2
43 LOAD_CONST               5 ('address')
46 LOAD_CONST               2 ('0001_initial')
49 BUILD_TUPLE              2
52 BUILD_TUPLE              4

@myint
Copy link
Collaborator

myint commented Nov 24, 2012

Comparing the AST works better.

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

No branches or pull requests

2 participants