Skip to content

Add xfail tests for case where specifications added as an extra are not honoured #8059

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 1 commit into from
May 3, 2020

Conversation

pelson
Copy link
Contributor

@pelson pelson commented Apr 16, 2020

Adds expected failing test cases for #7096.

Essentially pip ignores specifications in the extras if the package has already been specified in the install_requires. #7096 is essentially the same issue, but in that case it doesn't have any install_requires at all, and instead has two distinct but overlapping requirements in the extras.

To be clear: I'm happy for this to not be merged - it would be completely reasonable to have concerns about adding quite so many functional tests (4), each of which comes with a significant execution overhead. I'm happy to receive guidance on where might be a better place for unit-testing of this code.

No news entry added.

'{pkga_path}{extra_to_install}'.format(**locals()),
)

assert ('Successfully installed pkga-0.1 simple-{}'.format(simple_version)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@uranusjr
Copy link
Member

Thanks for the work. The test cases look good to me, but I would prefer putting it in test_new_resolver.py instead, and only run it against the new resolver. The current (“legacy”) implementation is unlikely to receive much work on functional deficiencies, especially this particular issue (I know because I worked on it and gave up). It’s not particularly useful to apply these tests for them.

@uranusjr uranusjr added the skip news Does not need a NEWS file entry (eg: trivial changes) label Apr 16, 2020
@pradyunsg pradyunsg self-requested a review April 26, 2020 14:36
@pradyunsg
Copy link
Member

@pelson Thanks for the PR! ^>^

Would it be possible for you to update the PR, or would your prefer that someone pick this up from here?

@pradyunsg
Copy link
Member

pradyunsg commented Apr 27, 2020

Actually, this is covered by https://github.com/pypa/pip/blob/a0fe4112d15ebcb20052c33f8792f237e1ad83e7/tests/yaml/install/extras.yml in our test suite currently, so I'm going to go ahead and close this PR.

@pradyunsg pradyunsg closed this Apr 27, 2020
@pradyunsg
Copy link
Member

OH WAIT. I misjudged this -- we aren't checking root requirements in that test case, which is something that's being done here.

@pradyunsg pradyunsg reopened this Apr 27, 2020
@pfmoore pfmoore merged commit d3847e3 into pypa:master May 3, 2020
@pelson pelson deleted the extra_requirements branch July 2, 2020 15:52
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants