Skip to content

gh-130149: refactor tests for HMAC #130150

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 27 commits into from
Mar 3, 2025
Merged

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Feb 15, 2025

  • test the repr() of HMAC OpenSSL objects properly.

@picnixz
Copy link
Member Author

picnixz commented Feb 15, 2025

This PR won't work against refleaks due to #130151.

@picnixz picnixz marked this pull request as ready for review February 15, 2025 12:29
@picnixz picnixz marked this pull request as draft February 24, 2025 17:51
@picnixz picnixz added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Mar 1, 2025
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @picnixz for commit 644f577 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F130150%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Mar 1, 2025
@picnixz picnixz added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Mar 2, 2025
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @picnixz for commit e71f03d 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F130150%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Mar 2, 2025
@picnixz picnixz marked this pull request as ready for review March 2, 2025 12:00
Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

While I'm not a fan of a giant maze of mixin classes from a code clarify point of view (nobody reading code can rationally follow a non-trivial class hierarchy MRO graph in their head)... this refactor should actually work and allow for easier testing of our too many possible implementation choices - and I don't have meaningfully better ideas for that that'd work within the stdlib test suite.

@picnixz
Copy link
Member Author

picnixz commented Mar 3, 2025

To ease usage in the future (although you or I will be the one reading this one with IntelliSense from IDE), I've added comments

I previously allowed it to be None for future compatibility, but
I think it's better to construct a fake hash function instead, or
@picnixz picnixz merged commit 8f11af4 into python:main Mar 3, 2025
38 checks passed
@picnixz picnixz deleted the test/hmac/refactor-99108 branch March 3, 2025 10:22
@picnixz
Copy link
Member Author

picnixz commented Mar 3, 2025

Arf, again I forgot about the build bots that don't have the built-in implementations. I'll do something for that so that we can cleanly skip the test.

seehwan pushed a commit to seehwan/cpython that referenced this pull request Apr 16, 2025
Since we plan to introduce a built-in implementation for HMAC based on HACL*,
it becomes important for the HMAC tests to be flexible enough to avoid code
duplication.

In addition to the new layout based on mixin classes, we extend test coverage by
also testing the `__repr__` of HMAC objects and the HMAC one-shot functions.

We also fix the import to `_sha256` which, since pythongh-101924, resulted in some tests being
skipped as the module is no more available (its content was moved to the `_sha2` module).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants