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

gh-130428: Add tests for delattr suggestions #130455

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

Copy link

cpython-cla-bot bot 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.

if operation == "getattr":
actual = self.get_suggestion(obj, 'bluch')
elif operation == "delattr":
actual = self.get_suggestion(lambda: delattr(obj, 'bluch'))
Copy link
Member

Choose a reason for hiding this comment

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

I would also add an else branch saying that the operation is not recognized otherwise.

Copy link
Author

Choose a reason for hiding this comment

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

Done! Kindly check the PR.

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

3 participants