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

Add tool to keep test-requirements in sync with pre-commit #3234

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

CoolCat467
Copy link
Member

In this pull request, we add a local pre-commit hook tool to keep the tool versions in test-requirements.txt in sync with the equivalent versions in .pre-commit-config.yaml automatically, meaning pre-commit.ci automatic updates are more seamless and there can never be cases where the test versions don't match the pre-commit versions.

@CoolCat467 CoolCat467 added project meta skip newsfragment Newsfragment is not required labels Mar 28, 2025
@CoolCat467 CoolCat467 requested a review from A5rocks March 28, 2025 04:37
Copy link

codecov bot commented Mar 28, 2025

Codecov Report

Attention: Patch coverage is 85.36585% with 12 lines in your changes missing coverage. Please review.

Project coverage is 56.11736%. Comparing base (5ce853c) to head (aed1deb).

Files with missing lines Patch % Lines
src/trio/_tools/sync_requirements.py 78.57143% 10 Missing and 2 partials ⚠️
Additional details and impacted files
@@                 Coverage Diff                 @@
##                main       #3234         +/-   ##
===================================================
- Coverage   56.17606%   56.11736%   -0.05871%     
===================================================
  Files            246         250          +4     
  Lines          37532       37696        +164     
  Branches        2538        2556         +18     
===================================================
+ Hits           21084       21154         +70     
- Misses         16320       16412         +92     
- Partials         128         130          +2     
Files with missing lines Coverage Δ
src/trio/_tests/tools/test_sync_requirements.py 100.00000% <100.00000%> (ø)
src/trio/_tools/sync_requirements.py 78.57143% <78.57143%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

Thanks this makes sense and seems pretty simple. We could probably make it simpler too.

I wonder whether maybe this should update the pre-commit configuration too, to maximize the version (lexicographically? Or just import packaging) between both files.

Another helpful related script might be to copy the pins from the requirements.txt files to the pre-commit file.

changed = False
old_lines = requirements.read_text(encoding="utf-8").splitlines(True)

with requirements.open("w", encoding="utf-8") as file:
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of feel like a loop over the version_data dict with a regex replacement might be simpler. Though this is already plenty simple.

# Get tool versions from pre-commit
# Get correct names
pre_commit_versions = {
name.removesuffix("-mirror").removesuffix("-pre-commit"): version
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should just do a dict of url to package name at the top instead of trying to automatically detect it

print("Changed test requirements version to match pre-commit")
print(f"{name}=={old_version} -> {name}=={version}")
file.write(new_line)
return changed
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also check whether every entry in version_data was used

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't immediately understand why this should be the case. For example, typos exists only as a pre-commit hook, and in fact I don't know if it even exists as a python package. Similar with zizmor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this would only make sense if #3234 (comment) was implemented.

@CoolCat467
Copy link
Member Author

I wonder whether maybe this should update the pre-commit configuration too, to maximize the version (lexicographically? Or just import packaging) between both files.

Another helpful related script might be to copy the pins from the requirements.txt files to the pre-commit file.

While this could be useful in some cases, I don't see a need for that in a regular use case. From my point of view, most of the time someone (or pre-commit.ci) would run pre-commit autoupdate and then uv pip compile --universal --python-version=3.9 test-requirements.in -o test-requirements.txt --upgrade, and then since all the pre-commit hook versions are either the original package itself or a mirror, they should have an identical version.

The primary use case for this script is so that when pre-commit ci automatically updates everything, this will automate pinning the test requirements to the same version.

Another possible solution that would sidestep needing this tool in the first place would be to just have pre-commit ci perform an upgrade on all the test requirements as well, but I don't think there is a way to specify a pre-commit hook to run that only runs on pre-commit.ci but not locally, unless one were to write a script to somehow identify something in the pre-commit.ci environment that is different from a local run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project meta skip newsfragment Newsfragment is not required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants