Skip to content

gh-77465: Increase test coverage for numbers #111738

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

Merged
merged 15 commits into from
Jan 25, 2024

Conversation

aisk
Copy link
Contributor

@aisk aisk commented Nov 4, 2023

Most classes in numbers are ABCs, but they have default implementations for some methods, for example, Complex.__bool__. Although the impementations are simple, but they should have been tested.

@bedevere-app bedevere-app bot added awaiting review tests Tests in the Lib/test dir labels Nov 4, 2023
@erlend-aasland erlend-aasland changed the title gh-77465: add more tests for numbers gh-77465: Increase test coverage for numbers Jan 6, 2024
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

There is too many code for implementing methods that are not used, just to allow instantiation of the subclass of an abstract class. I suggest to generate them.

import abc

def not_implemented(*args, **kwargs):
    raise NotImplementedError

def concretize(cls):
    for name in dir(cls):
        try:
            value = getattr(cls, name)
            if value.__isabstractmethod__:
                setattr(cls, name, not_implemented)
        except AttributeError:
            pass
    abc.update_abstractmethods(cls)
    return cls

You can just apply this decorator to subclasses and remove all dummy methods.

@aisk
Copy link
Contributor Author

aisk commented Jan 19, 2024

Thanks for the review, applied!

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

@serhiy-storchaka serhiy-storchaka merged commit e721adf into python:main Jan 25, 2024
@miss-islington-app
Copy link

Thanks @aisk for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 25, 2024
…GH-111738)

(cherry picked from commit e721adf)

Co-authored-by: AN Long <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 25, 2024
…GH-111738)

(cherry picked from commit e721adf)

Co-authored-by: AN Long <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jan 25, 2024

GH-114556 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Jan 25, 2024
@bedevere-app
Copy link

bedevere-app bot commented Jan 25, 2024

GH-114557 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Jan 25, 2024
serhiy-storchaka added a commit that referenced this pull request Jan 25, 2024
…1738) (GH-114557)

(cherry picked from commit e721adf)

Co-authored-by: AN Long <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>
@aisk aisk deleted the test-numbers branch January 25, 2024 15:33
serhiy-storchaka added a commit that referenced this pull request Jan 25, 2024
…1738) (GH-114556)

(cherry picked from commit e721adf)

Co-authored-by: AN Long <[email protected]>
Co-authored-by: Serhiy Storchaka <[email protected]>
aisk added a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
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