Skip to content

gh-130428: Add tests for delattr suggestions #130455

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Pranjal095
Copy link

@Pranjal095 Pranjal095 commented Feb 22, 2025

Depends on #130427.

With reference to the enhancement proposed in issue #130425, tests have been added to ensure proper functionality of delattr suggestions. To maintain consistency and readability of the code, they mirror the tests for getattr suggestions existing in the modified file (Lib/test/test_traceback.py).

@ghost
Copy link

ghost commented Feb 22, 2025

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Feb 22, 2025

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

actual = self.get_suggestion(lambda: delattr(obj, 'somethingverywrong'))
self.assertNotIn("blech", actual)

def test_delattr_error_bad_suggestions_do_not_trigger_for_small_names(self):
Copy link
Member

Choose a reason for hiding this comment

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

This test is the same as for getattr, can't it be refactored so that we use the same fixtures but with different calls to self.get_suggestion?

Copy link
Author

Choose a reason for hiding this comment

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

I was actually thinking of doing that, but decided to go with duplication for better readability.
On reconsideration with your review, I believe it is redundant and should be refactored. Will update the PR accordingly.

@tomasr8
Copy link
Member

tomasr8 commented Feb 23, 2025

@Pranjal095 Some of the tests you added are failing, could you fix them please?

@Pranjal095
Copy link
Author

@tomasr8 The reason for the proposed tests failing is that the pull request implementing name suggestions for delattr is yet to be merged into the main branch. Pull request linked here.
Locally, I ran the tests with the pull request changes and they all passed.

@picnixz
Copy link
Member

picnixz commented Feb 23, 2025

Let's put it as a draft until #130427 is merged then.

@picnixz picnixz marked this pull request as draft February 23, 2025 12:44
@encukou
Copy link
Member

encukou commented Mar 3, 2025

Usually we'd merge a new feature together with its tests. Could you combine the PRs into one?

Pranjal095 added a commit to Pranjal095/cpython that referenced this pull request Mar 3, 2025
…gestions-combined

This merges the delattr suggestion feature implementation by @sobolevn from PR python#130427 with added tests from PR python#130455.
@Pranjal095
Copy link
Author

I've made a new PR combining the commits from both corresponding forks. Kindly let me know if there are any further modifications to be made.

@python-cla-bot
Copy link

python-cla-bot bot commented Apr 18, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

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.

4 participants