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

arthur-github-user · 2017-02-21T15:02:42Z

Add unit tests for find_function, getsourcelines and lasti2lineno functions.

the-knights-who-say-ni · 2017-02-21T15:02:43Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

If you don't have an account on b.p.o, please create one
Make sure your GitHub username is listed in "Your Details" at b.p.o
If you have not already done so, please sign the PSF contributor agreement
If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

arthur-github-user · 2017-02-22T08:50:31Z

All steps listed above completed

arthur-github-user · 2017-02-22T09:18:33Z

@Haypo @serhiy-storchaka @methane please review this PR

methane · 2017-02-22T09:46:24Z

Before review, why did you added test for private utility functions?
They are not covered other tests for higher level functions? Do you want cover
edge case of them which cannot test from higher level functions?

arthur-github-user · 2017-02-22T09:54:24Z

@methane, because of edge cases. Also it seems easier to write such whitebox tests for small isolated units(and this is what documentation suggests if I understood correctly). Generally I want to write tests for the whole module and because there will be quite a few tests I decided to split it into multiple small PRs.

methane · 2017-02-22T10:10:10Z

Lib/test/test_pdb.py

+
+    def test_getsourcelines_with_module_frame_obj(self):
+        for frame_tuple in inspect.getouterframes(inspect.currentframe()):
+            frame = frame_tuple[0]


for frame, *unused in inspect.getouterframes(inspect.currentframe()):

methane · 2017-02-22T10:11:50Z

Lib/test/test_pdb.py

+            frame = frame_tuple[0]
+            if frame.f_globals is frame.f_locals:
+                module_frame = frame
+                break


I don't like this hack to get frame object for module...
But I don't know is there any way.
I'm not expert of inspect and pdb.

methane · 2017-02-22T10:15:01Z

Lib/test/test_pdb.py

+        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)


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?

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()).

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.

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

@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 ?

@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".

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

methane · 2017-02-22T10:18:40Z

OK. I leave some comments.

But I think I'm not suited to this change, because this code uses some inspect magic and
I'm not familiar with inspect and pdb module.
I don't know is there any better way other than than proposed magic.

arthur-github-user · 2017-02-22T10:24:07Z

@methane thank you very much for comments, can you please suggest person who can review test for pdb?

methane · 2017-02-22T10:29:49Z

I usually use Experts Index and git history to find experts.

Experts Index doesn't tell expert of pdb. But @1st1 is listed as expert of inspect module.
As reading history of pdb, @birkenfeld did very important jobs for pdb.

methane · 2017-02-22T10:42:32Z

And there are some comments in b.p.o

methane · 2017-02-28T07:21:18Z

While pdb and inspect relying on implementation, PyPy is compatible at this point.
So I don't think cpython_only is required for this, until Python 3.6 implementation
which has incompatible code object born.

methane · 2017-02-28T07:24:17Z

Lib/test/test_pdb.py

+        self.assertEqual(actual_lineno, expected_lineno)
+
+    def test_getsourcelines_with_module_frame_obj(self):
+        for frame, *unused in inspect.getouterframes(inspect.currentframe()):


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

But documentation says that whitebox tests are preferred

"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.

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.

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?

frame = inspect.currentframe()

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

It won't be module frame.

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.

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?

…dded SLP - Fix test_outside to reset cstack_base and cstack_root. Now the simulation of a call from outside is complete. - Add test_slp_embed to test embedding Stackless

…terpreter The fix for Stackless issue 186 (support for sub-interpreters) broke some typical use cases.

…dded SLP - Fix test_outside to reset cstack_base and cstack_root. Now the simulation of a call from outside is complete. - Add test_slp_embed to test embedding Stackless. Currently it gets skipped, because it requires a not yet available class.

…terpreter The fix for Stackless issue 186 (support for sub-interpreters) broke some typical use cases. (cherry picked from commit 5b29b24)

the-knights-who-say-ni added the CLA not signed label Feb 21, 2017

Mariatta removed the CLA not signed label Feb 21, 2017

the-knights-who-say-ni added the CLA signed label Feb 21, 2017

arthur-github-user force-pushed the bpo-29598-add-unit-tests-for-pdb branch from 2a5a46d to 3479961 Compare February 22, 2017 08:58

methane reviewed Feb 22, 2017

View reviewed changes

bpo-29598 Add couple of unit tests for pdb module

fcf80b0

arthur-github-user force-pushed the bpo-29598-add-unit-tests-for-pdb branch from 3479961 to fcf80b0 Compare February 22, 2017 13:49

berkerpeksag requested a review from xdegaye February 23, 2017 07:48

methane reviewed Feb 28, 2017

View reviewed changes

arthur-github-user closed this Mar 16, 2017

akruis pushed a commit to akruis/cpython that referenced this pull request Aug 4, 2019

Stackless issue python#218: Fix using Stackless as embedded Python in…

5b29b24

…terpreter The fix for Stackless issue 186 (support for sub-interpreters) broke some typical use cases.

micgeronimo mannequin mentioned this pull request Apr 10, 2022

Write unit tests for pdb module #73784

Closed

jaraco pushed a commit that referenced this pull request Dec 2, 2022

Use Python 3.6.8 runtime (#218)

f6722b5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

arthur-github-user commented Feb 21, 2017

the-knights-who-say-ni commented Feb 21, 2017

arthur-github-user commented Feb 22, 2017

arthur-github-user commented Feb 22, 2017

methane commented Feb 22, 2017

arthur-github-user commented Feb 22, 2017

methane Feb 22, 2017

methane Feb 22, 2017

methane Feb 22, 2017

xdegaye Feb 24, 2017

methane Feb 25, 2017

xdegaye Feb 26, 2017

xdegaye Feb 26, 2017

methane Feb 28, 2017

methane Feb 28, 2017

methane commented Feb 22, 2017 •

edited

Loading

arthur-github-user commented Feb 22, 2017

methane commented Feb 22, 2017 •

edited

Loading

methane commented Feb 22, 2017

methane commented Feb 28, 2017

methane Feb 28, 2017

arthur-github-user Feb 28, 2017

methane Feb 28, 2017

methane Feb 28, 2017

arthur-github-user Feb 28, 2017 •

edited

Loading

methane Feb 28, 2017

arthur-github-user Feb 28, 2017

methane Feb 28, 2017

arthur-github-user Feb 28, 2017

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

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

Conversation

arthur-github-user commented Feb 21, 2017

the-knights-who-say-ni commented Feb 21, 2017

arthur-github-user commented Feb 22, 2017

arthur-github-user commented Feb 22, 2017

methane commented Feb 22, 2017

arthur-github-user commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

methane commented Feb 22, 2017 • edited Loading

arthur-github-user commented Feb 22, 2017

methane commented Feb 22, 2017 • edited Loading

methane commented Feb 22, 2017

methane commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

methane commented Feb 22, 2017 •

edited

Loading

methane commented Feb 22, 2017 •

edited

Loading

arthur-github-user Feb 28, 2017 •

edited

Loading