Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

archy-rock3t-cloud
Copy link

Add unit tests forfind_function,getsourcelines andlasti2lineno functions.

@the-knights-who-say-ni

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 thePSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username onbugs.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, pleasecreate 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 thePSF contributor agreement
  4. If you just signed the CLA, pleasewait at least one US business day and then check "Your Details" onbugs.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-rock3t-cloud
Copy link
Author

All steps listed above completed

@archy-rock3t-cloudarchy-rock3t-cloudforce-pushed thebpo-29598-add-unit-tests-for-pdb branch from2a5a46d to3479961CompareFebruary 22, 2017 08:58
@archy-rock3t-cloud
Copy link
Author

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

@methane
Copy link
Member

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-rock3t-cloud
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 thenormative 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 commentedFeb 22, 2017
edited
Loading

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-rock3t-cloud
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 commentedFeb 22, 2017
edited
Loading

I usually useExperts 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 readinghistory of pdb,@birkenfeld did very important jobs for pdb.

@methane
Copy link
Member

And there are some comments in b.p.o

@methane
Copy link
Member

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.

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 oftest target, nottest runner.

What ismodule_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 framein 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

@archy-rock3t-cloudarchy-rock3t-cloudFeb 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.

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.

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 requestAug 4, 2019
…dded SLP- Fix test_outside to reset cstack_base and cstack_root. Now thesimulation 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 requestAug 4, 2019
…terpreterThe fix for Stackless issue 186 (support for sub-interpreters) brokesome typical use cases.
akruis pushed a commit to akruis/cpython that referenced this pull requestMay 27, 2021
…dded SLP- Fix test_outside to reset cstack_base and cstack_root. Now thesimulation of a call from outside is complete.- Add test_slp_embed to test embedding Stackless. Currently it getsskipped, because it requires a not yet available class.
akruis pushed a commit to akruis/cpython that referenced this pull requestMay 27, 2021
…terpreterThe fix for Stackless issue 186 (support for sub-interpreters) brokesome typical use cases.(cherry picked from commit5b29b24)
@micgeronimomicgeronimomannequin mentioned this pull requestApr 10, 2022
jaraco pushed a commit that referenced this pull requestDec 2, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@methanemethanemethane left review comments

@xdegayexdegayeAwaiting requested review from xdegaye

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@archy-rock3t-cloud@the-knights-who-say-ni@methane@xdegaye@Mariatta

[8]ページ先頭

©2009-2025 Movatter.jp