Skip to content

bpo-32416: Refactor tests for the f_lineno setter and add new tests. #4991

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

Merged
merged 5 commits into from
Dec 26, 2017

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Dec 23, 2017

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Very nice. A couple small comments are inline.

@@ -5,6 +5,18 @@
import sys
import difflib
import gc
from functools import wraps

class tracecontext:
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a docstring here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

raise
else:
# Something's wrong - the expected exception wasn't raised.
raise RuntimeError("Trace-function-less jump failed to fail")
Copy link
Member

Choose a reason for hiding this comment

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

AssertionError perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't touched this function. Agreed, AssertionError looks more appropriate here.

sys.settrace(None)
self.compare_jump_output(expected, output)

def jump_test(jumpFrom, jumpTo, expected, error=None):
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a docstring perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

def decorator(func):
@wraps(func)
def test(self):
self.run_test(func, jumpFrom+1, jumpTo+1, expected, error)
Copy link
Member

Choose a reason for hiding this comment

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

Why +1?

Copy link
Member Author

Choose a reason for hiding this comment

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

To compensate a decorator line.

pass
jump_across_with.jump = (1, 3)
jump_across_with.output = []
@jump_test(5, 7, [4], (ValueError, 'into'))
Copy link
Member

Choose a reason for hiding this comment

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

Why is this forbidden even though test_jump_between_except_blocks is ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because except with a variable creates an implicit finally block.

else:
# Something's wrong - the expected exception wasn't raised.
raise RuntimeError("Trace-function-less jump failed to fail")
@jump_test(3, 5, [2, 5], (ValueError, 'finally'))
Copy link
Member

Choose a reason for hiding this comment

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

If jumping is forbidden, then I'm curious why output.append(3) is not executed?

Edit: oh, I see... the jump raises ValueError so still jumps to the finally block. Perhaps add a comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

output.append(5)

@jump_test(3, 5, [1, 2, -2], (ValueError, 'into'))
def test_jump_across_with_2(output):
Copy link
Member

Choose a reason for hiding this comment

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

It's really the jumping into a with block that's forbidden, right? Jumping out of it seems ok...

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. There was test_jump_across_with which looked strangely so I have added test_jump_across_with_2 just for the case of an error in the former.

Actually test_jump_across_with may be working as intended. Once it caught a bug in my code when most of other tests were passed.

test_jump_across_with_2 itself can catch other bug. Both source and destination have the same level of nesting. Just they are nested in different blocks.

Thus I'll keep both tests.

self.fail( "Outputs don't match:\n" +
"Expected: " + repr(expected) + "\n" +
"Received: " + repr(received))
def test_no_jump_to_non_integers(self):
Copy link
Member

Choose a reason for hiding this comment

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

Those two tests should go after the @jump_test functions, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

jump_test doesn't support non-integer arguments.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I mean this test should go after the tests decorated with @jump_test.

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thank you @serhiy-storchaka !

@serhiy-storchaka serhiy-storchaka merged commit 53f9135 into python:master Dec 26, 2017
@miss-islington
Copy link
Contributor

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6.
🐍🍒⛏🤖

@serhiy-storchaka serhiy-storchaka deleted the test-jump branch December 26, 2017 22:39
@miss-islington
Copy link
Contributor

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 53f9135667226f33e049e327db60fb033afbd77a 3.6

@miss-islington
Copy link
Contributor

Sorry, @serhiy-storchaka, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 53f9135667226f33e049e327db60fb033afbd77a 2.7

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Dec 27, 2017
@bedevere-bot
Copy link

GH-5016 is a backport of this pull request to the 3.6 branch.

@bedevere-bot
Copy link

GH-5017 is a backport of this pull request to the 2.7 branch.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Dec 27, 2017
serhiy-storchaka added a commit that referenced this pull request Dec 27, 2017
serhiy-storchaka added a commit that referenced this pull request Dec 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants