Skip to content

gh-130428: Add tests for delattr suggestions#130455

Closed
Pranjal095 wants to merge 4 commits into
python:mainfrom
Pranjal095:delattr-suggestion-tests
Closed

gh-130428: Add tests for delattr suggestions#130455
Pranjal095 wants to merge 4 commits into
python:mainfrom
Pranjal095:delattr-suggestion-tests

Conversation

@Pranjal095

@Pranjal095 Pranjal095 commented Feb 22, 2025

Copy link
Copy Markdown
Contributor

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

ghost commented Feb 22, 2025

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app

bedevere-app Bot commented Feb 22, 2025

Copy link
Copy Markdown

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.

Comment thread Lib/test/test_traceback.py Outdated
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
Copy Markdown
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
Copy Markdown
Contributor 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.

Comment thread Lib/test/test_traceback.py
@tomasr8

tomasr8 commented Feb 23, 2025

Copy link
Copy Markdown
Member

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

@Pranjal095

Copy link
Copy Markdown
Contributor 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

picnixz commented Feb 23, 2025

Copy link
Copy Markdown
Member

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

encukou commented Mar 3, 2025

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor 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

python-cla-bot Bot commented Apr 18, 2025

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@Pranjal095 Pranjal095 closed this Aug 22, 2025
@Pranjal095 Pranjal095 deleted the delattr-suggestion-tests branch August 22, 2025 14:05
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