-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Remove astor and reproduce the original assertion expression #5512
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5512 +/- ##
==========================================
- Coverage 96.09% 96.06% -0.03%
==========================================
Files 117 117
Lines 25647 25697 +50
Branches 2483 2494 +11
==========================================
+ Hits 24646 24687 +41
- Misses 696 702 +6
- Partials 305 308 +3
Continue to review full report at Codecov.
|
@@ -285,7 +287,7 @@ def _rewrite_test(fn, config): | |||
with open(fn, "rb") as f: | |||
source = f.read() | |||
tree = ast.parse(source, filename=fn) | |||
rewrite_asserts(tree, fn, config) | |||
rewrite_asserts(tree, source, fn, config) |
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.
Not that this is a blocker, but this will end up breaking https://pypi.org/project/pytest-ast-back-to-python.
cc @tomviner
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.
ah, I can restore the api and just re-read the source in the visitor using fn
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.
Just to update here, pytest-ast-back-to-python has now been fixed.
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.
Awesome Tom, thanks for the heads up!
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.
Thanks a lot for working on this @asottile!
I feel a little off in that we might be reinventing the wheel, but if astor
does not provide the functionality we need correctly, we might need to implement our own, thanks for that. 👍
@@ -1371,10 +1371,21 @@ def test_fails(): | |||
) | |||
result = testdir.runpytest() | |||
result.stdout.fnmatch_lines( | |||
"*Assertion Passed: a + b == c + d (1 + 2) == (3 + 0) at line 7*" | |||
"*Assertion Passed: a+b == c+d (1 + 2) == (3 + 0) at line 7*" |
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.
Did you see the approach I took in #5511 of using inline_run
and checking the parameters received by the hook directly? Feels more elegant that way as we can check the parameters directly instead of relying on parsed output.
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.
I did yeah, didn't quite feel like expanding the api scope of pytester further but I can take another look at that again 👍
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.
I added a two-liner helper function to ParsedCall
, that's all. I think it is an improvement.
Anyway just a suggestion really. 👍
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.
Awesome, thanks!
alrighty, let me replay the release branch :D |
Resolves #5510
This is a little different approach than using
astor
in that it reproduces the original assertion source expression directlyI could make this even simpler by including the whole line of the original assertion (then the indentation would be preserved as well???) what do you think of that?