Skip to content

Conditionalize more tests that require PyPI #13699

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 3 commits into from
Jun 2, 2025

Conversation

musicinmybrain
Copy link
Contributor

Summary

Use the existing pypi feature to conditionalize a number of tests that attempt to access https://pypi.org and/or https://files.pythonhosted.org. See #8970 (comment).

There is no reason to believe that these are all of the tests that need to be conditionalized on the pypi feature, but this should be a solid step in the right direction.

Test Plan

This allows me to build and run the integration tests in Fedora’s uv package without having to manually skip tests that try to access PyPI. I confirmed that this appears to accomplish that goal.

Otherwise, this should be tested by building and running the tests as usual. As mentioned in #8970 (comment), a more complete solution would include CI tests that confirm these features are working as intended. I’m not in a position to offer that.

@konstin
Copy link
Member

konstin commented May 28, 2025

For pip_check.rs and workspace.rs, should we gate the entire modules instead?

@musicinmybrain
Copy link
Contributor Author

musicinmybrain commented May 28, 2025

For pip_check.rs and workspace.rs, should we gate the entire modules instead?

For pip_check.rs, sure, assuming you feel that it will most likely continue to contain only PyPI-requiring tests. While I haven’t tried to make the python feature work as it should, I suspect that pip_check should probably require it as well, like most of the existing conditionals:

#[cfg(all(feature = "python", feature = "pypi"))]
mod pip_sync;

For workspace.rs, a number of tests do seem to be able to run without network access and therefore without PyPI: at least workspace_to_workspace_paths_dependencies, workspace_empty_member, workspace_hidden_files, workspace_hidden_member, workspace_non_included_member, workspace_inherit_sources, and test_path_hopping. (I didn’t conditionalize transitive_dep_in_git_workspace_no_root and transitive_dep_in_git_workspace_with_root on pypi because the git feature is disabled for my offline builds, so I couldn’t trivially evaluate whether or not they also needed pypi.)

@musicinmybrain
Copy link
Contributor Author

I see also that I made an error in crates/uv-client/tests/it/remote_metadata.rs:

error: unexpected `cfg` condition value: `pypi`
  --> crates/uv-client/tests/it/remote_metadata.rs:13:7
   |
13 | #[cfg(feature = "pypi")]
   |       ^^^^^^^^^^^^^^^^ help: remove the condition
   |
   = note: no expected values for `feature`
   = help: consider adding `pypi` as a feature in `Cargo.toml`
   = note: see <https://doc.rust-lang.org/nightly/rustc/check-cfg/cargo-specifics.html> for more information about checking conditional configuration
   = note: `-D unexpected-cfgs` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(unexpected_cfgs)]`

This does need to be conditionalized on PyPI access somehow, but the pypi feature is available only in the uv crate, not in uv-client. I think that puts that particular test beyond the scope of this PR, and I’ll need to keep explicitly skipping that test downstream for now.

@konstin
Copy link
Member

konstin commented May 28, 2025

We can expose the a pypi feature in uv-client too and activate it from the uv feature to handle that case.

@musicinmybrain
Copy link
Contributor Author

Rebased on main without other changes.

Copy link

codspeed-hq bot commented May 31, 2025

CodSpeed Walltime Performance Report

Merging #13699 will improve performances by 15.79%

Comparing musicinmybrain:add-pypi-cfg (2d77f60) with main (13a86a2)

Summary

⚡ 1 improvements
✅ 11 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
wheelname_parsing_failure[flyte-long-extension] 110 ns 95 ns +15.79%

@konstin konstin merged commit 73829f3 into astral-sh:main Jun 2, 2025
86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants