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

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

Conversation

archy-devops
Copy link

Add unit tests for find_function, getsourcelines and lasti2lineno functions.

@the-knights-who-say-ni
Copy link

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:

  1. If you don't have an account on b.p.o, please create one
  2. Make sure your GitHub username is listed in "Your Details" at b.p.o
  3. If you have not already done so, please sign the PSF contributor agreement
  4. 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)
  5. Reply here saying you have completed the above steps

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

@archy-devops
Copy link
Author

All steps listed above completed

@archy-devops archy-devops force-pushed the bpo-29598-add-unit-tests-for-pdb branch from 2a5a46d to 3479961 Compare February 22, 2017 08:58
@archy-devops
Copy link
Author

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

@methane
Copy link
Member

methane commented Feb 22, 2017

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?

@archy-devops
Copy link
Author

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


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

Choose a reason for hiding this comment

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

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

frame = frame_tuple[0]
if frame.f_globals is frame.f_locals:
module_frame = frame
break
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 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.

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.

@methane
Copy link
Member

methane commented Feb 22, 2017

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.

@archy-devops
Copy link
Author

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

@methane
Copy link
Member

methane commented Feb 22, 2017

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
Copy link
Member

methane commented Feb 22, 2017

And there are some comments in b.p.o

@methane
Copy link
Member

methane commented Feb 28, 2017

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.

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

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?

akruis pushed a commit to akruis/cpython that referenced this pull request Aug 4, 2019
…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
akruis pushed a commit to akruis/cpython that referenced this pull request Aug 4, 2019
…terpreter

The fix for Stackless issue 186 (support for sub-interpreters) broke
some typical use cases.
akruis pushed a commit to akruis/cpython that referenced this pull request May 27, 2021
…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.
akruis pushed a commit to akruis/cpython that referenced this pull request May 27, 2021
…terpreter

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

(cherry picked from commit 5b29b24)
@micgeronimo micgeronimo mannequin mentioned this pull request Apr 10, 2022
jaraco pushed a commit that referenced this pull request Dec 2, 2022
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

Successfully merging this pull request may close these issues.

None yet

5 participants