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 pip_cert pytest fixture #2083

Merged
merged 1 commit into from
Mar 17, 2021
Merged

Conversation

frenzymadness
Copy link
Contributor

pip_cert is recognized as a generator so it should not use
return if PIP_CERT is in os.environ.

Fixes: #2048

  • ran the linter to address style issues (tox -e fix_lint)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

I'm not sure this is testable easily given the fact that a CA cert bundle might be in various locations. Is a change in tests worth mentioning in the changelog?

Before this change:

$ PIP_CERT=/etc/ssl/certs/ca-bundle.crt tox -e py39 -- -x
…
ValueError: pip_cert did not yield a value

after this change:

$ PIP_CERT=/etc/ssl/certs/ca-bundle.crt tox -e py39 -- -x
…
  py39: commands succeeded
  congratulations :)

pip_cert is recognized as a generator so it should not use
return if PIP_CERT is in os.environ.

Fixes: pypa#2048
@codecov
Copy link

codecov bot commented Mar 17, 2021

Codecov Report

Merging #2083 (c082ee5) into main (bb446cb) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2083      +/-   ##
==========================================
- Coverage   93.72%   93.70%   -0.03%     
==========================================
  Files          88       88              
  Lines        4383     4383              
==========================================
- Hits         4108     4107       -1     
- Misses        275      276       +1     
Flag Coverage Δ
tests 93.70% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/virtualenv/util/path/_pathlib/__init__.py 93.93% <0.00%> (-3.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb446cb...c082ee5. Read the comment docs.

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Can you add some tests to this, please?

@frenzymadness
Copy link
Contributor Author

Can you add some tests to this, please?

Do you know about a universal CA bundle I can use across all the systems you have in CI? It needs to be set before pytest is executed so I can imagine a new tox environment running only some tests with this environment variable set.

Is it worth it given the fact that the whole fixture is just a workaround for a problem in pip and its author did not add tests for it?

I can also imagine some dummy test exercising just the unused part of the fixture and actually testing nothing.

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

@gaborbernat gaborbernat merged commit 133050c into pypa:main Mar 17, 2021
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.

pip_cert fixture breaks tests if PIP_CERT is in os.environ
2 participants