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.

Sign up for GitHub

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

bpo-29598 Add couple of unit tests for pdb module #218

Closed
wants to merge 1 commit into from
Closed

bpo-29598 Add couple of unit tests for pdb module #218

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions Lib/test/test_pdb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# A test suite for pdb; not very comprehensive at the moment.

import dis
import doctest
import inspect
import os
import pdb
import sys
Expand Down Expand Up @@ -1110,6 +1112,44 @@ def test_readrc_kwarg(self):
if save_home is not None:
os.environ['HOME'] = save_home

def test_find_function_with_invalid_filename(self):
self.assertIsNone(pdb.find_function('foo', 'invalid filename'))

def test_getsourcelines(self):
def foo():
pass # pragma: no cover
lines, lineno = inspect.findsource(foo)
expected_lines = inspect.getblock(lines[lineno:])
expected_lineno = lineno + 1
actual_lines, actual_lineno = pdb.getsourcelines(foo)
self.assertEqual(actual_lines, expected_lines)
self.assertEqual(actual_lineno, expected_lineno)

def test_getsourcelines_with_module_frame_obj(self):
for frame, *unused in inspect.getouterframes(inspect.currentframe()):
Copy link
Member

Choose a reason for hiding this comment

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

I don't like test relying on outer frame.
I like test which doesn't relying test runner implementation.

Copy link
Author

Choose a reason for hiding this comment

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

But documentation says that whitebox tests are preferred

Copy link
Member

Choose a reason for hiding this comment

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

"whitebox test" is relying on implementation of test target, not test runner.

What is module_frame here? Isn't it relying on test runner?
It can be changed even if pdb and test_pdb is not changed.

So I prefer creating module frame in this test, not capturing from caller of this test.

Copy link
Member

Choose a reason for hiding this comment

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

BTW, is there any difference between module frame and other frames from pdb's point of view?
If there is no difference, function frame is far easier to create, maybe.

Copy link
Author

@arthur-github-user arthur-github-user Feb 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

In this case I need specifically module frame(as you can see from pdb code itself) and it seems like it doesn't matter which module frame it will be. Can you please show how to create it manually?

Copy link
Member

Choose a reason for hiding this comment

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

frame = inspect.currentframe()

frame must be current test's frame. caller's frame shouldn't affects this test.

Copy link
Author

Choose a reason for hiding this comment

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

It won't be module frame.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I'm sorry. I misread "I need" as "I don't need".

Now I read pdb.getsourcelines().
If I'm correct, do you want to test this one line: return lines, 1?

I don't know way to create module frame which is simple enough to test such simple one line.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I would like it to be tested. Initially, when I've seen test coverage of stdlib I wanted to write some tests to improve it. Now it seems like tough task, because it is not even clear who should approve my PRs. If I address all comments here and in bpo, whom can I ask to approve my PR?

if frame.f_globals is frame.f_locals:
module_frame = frame
break
expected_lines = inspect.findsource(module_frame)[0]
lines, lineno = pdb.getsourcelines(module_frame)
self.assertEqual(lines, expected_lines)
self.assertEqual(lineno, 1)

def test_getsourcelines_with_module_obj(self):
module = inspect.getmodule(inspect.currentframe())
expected_lines = inspect.findsource(module)[0]
lines, lineno = pdb.getsourcelines(module)
self.assertEqual(lines, expected_lines)
self.assertEqual(lineno, 1)

def test_lasti2lineno(self):
code_obj = inspect.currentframe().f_code
max_offset, max_lineno = max(dis.findlinestarts(code_obj))
min_offset, min_lineno = min(dis.findlinestarts(code_obj))
self.assertEqual(pdb.lasti2lineno(code_obj, max_offset), max_lineno)
self.assertEqual(pdb.lasti2lineno(code_obj, max_offset+1), max_lineno)
self.assertEqual(pdb.lasti2lineno(code_obj, min_offset-1), 0)
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid this test is relying implementation detail.
Doesn't #46 affects this? Is this test passes on other Python implementations like PyPy or micropython?

Copy link
Contributor

Choose a reason for hiding this comment

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

What implementation detail ?
The Python implementations like PyPy follow also the normative description of co_lnotab. I may be wrong, but differences in the definitions of the structure of the AST between those implementations should not bear any impact on the definition of the structure of co_lnotab.

As a side note, offsets increase monotonically so min_offset is the first offset yielded by findlinestarts() and max_offset is the last one (no need to invoke min() or max()).

Copy link
Member

Choose a reason for hiding this comment

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

I didn't research deeply, I just worried. So if PyPy passes this, it's OK.

I think attributes of Python's code object is implementation detail, even though PyPy follows.
But it's not big problem until there are some Python implementation having different code implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have started a discussion on python-dev about attributes of Python code objects being implementation detail.

Copy link
Contributor

Choose a reason for hiding this comment

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

@methane
Assuming now that the specification of co_lnotab is an implementation detail, then the pdb and inspect modules from the CPython standard library may fail to run on a new Python implementation that does not follow the CPython co_lnotab specification. So why do you want to constrain test_pdb to run on this Python implementation when the pdb and inspect modules themselves would not ?

Copy link
Member

Choose a reason for hiding this comment

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

@xdegaye I didn't say "I want to do ...". I was asked to review, and leave comment what I feel.
I didn't say LGTM or "change required".

Copy link
Member

Choose a reason for hiding this comment

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

Please ignore me when my comment doesn't make sense for you.
I'm not familiar with this area.


def tearDown(self):
support.unlink(support.TESTFN)

Expand Down