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

Fix calculation of PipProvider._known_depths #10438

Closed
wants to merge 25 commits into from

Conversation

notatallshaw
Copy link
Member

Simple fix for #10415

I feel like we should add unit tests somewhere, but I can't find any examples or PipProvider being explicitly tested, am I missing something?

@uranusjr
Copy link
Member

uranusjr commented Sep 6, 2021

The resolver is currently only tested indirectly in integration tests (functional/test_new_resolver*) and not in unit tests. (Mainly because it’s freaking difficult to set up the objects to use in the provider so it’s easier to just call pip in a subprocess).

jdufresne and others added 11 commits September 8, 2021 05:37
setuptools removed use_2to3 in version v58.0.1 (2021-09-06). Some tests
install the anyjson package which uses this feature and so those tests
result in a command failure. The failure appeared as:

    error in anyjson setup command: use_2to3 is invalid.

As anyjson package is no longer maintained (last release was 2012),
change the tests to use a package that is healthy.

For details on the setuptools change, see the history at:

https://setuptools.readthedocs.io/en/latest/history.html#v58-0-2
Unused since 6b54145 when the last test
method was removed.

Removing the class avoids the need to add type annotations.
Replace assert_raises_regexp function with pytest.raises
Handle TODO comment in tests/functional/test_new_resolver.py
This now supports:

- Clearer failure mode, for imports of the format `import x.y`
- Regular expression support for dropping files (useful for when we vendor pygments)
- Patching files prior to rewriting imports, allowing generation of patches on the
  original package sources.
- Detection of py.typed files, which omits generation of unnecessary `.pyi` stubs.
@notatallshaw
Copy link
Member Author

The resolver is currently only tested indirectly in integration tests (functional/test_new_resolver*) and not in unit tests. (Mainly because it’s freaking difficult to set up the objects to use in the provider so it’s easier to just call pip in a subprocess).

Okay, so do you want me to add a test?

It's possible to make a test that builds a deep enough requirement tree where this commit picks a less deep package to resolve on than the current resolver and that this then creates a different installation. But I don't like that approach because it makes assumptions about what is the definition of "bad" behavior is, which could change in the future as people add optimizations and it would make the test difficult to update.

Are there any current tests which specifically try to test the behavior of get_preference?

P.S I don't know what's happened to my PR and why it now says it has 20 commits.

jdufresne and others added 4 commits September 13, 2021 18:38
The msgpack-python was replaced by msgpack. This removes the need to
special case its directory.

The pytoml library is no longer vendored.

The resolvelib package distributes its license and so doesn't require a
license fallback URL.
Simplify vendoring configuration
Remove unused class TestUpgradeDistributeToSetuptools
@notatallshaw
Copy link
Member Author

I don't know why PR is broken, and every attempt I've tried to fix it has made it worse. I am closing it and making a new one.

@notatallshaw notatallshaw deleted the fix_known_depths branch September 16, 2021 18:30
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants