Skip to content

Fix pip_cert pytest fixture#2083

Merged
gaborbernat merged 1 commit intopypa:mainfrom
frenzymadness:fix_pip_cert
Mar 17, 2021
Merged

Fix pip_cert pytest fixture#2083
gaborbernat merged 1 commit intopypa:mainfrom
frenzymadness:fix_pip_cert

Conversation

@frenzymadness
Copy link
Copy Markdown
Contributor

@frenzymadness frenzymadness commented Mar 17, 2021

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 :)

@frenzymadness
Fix pip_cert pytest fixture
c082ee5 
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
Copy Markdown

codecov bot commented Mar 17, 2021 •
edited
Loading

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.

gaborbernat
gaborbernat requested changes Mar 17, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

frenzymadness commented Mar 17, 2021

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.

gaborbernat
gaborbernat approved these changes Mar 17, 2021
View reviewed changes
Copy link
Copy Markdown
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

Reviewers

@gaborbernat gaborbernat gaborbernat approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

pip_cert fixture breaks tests if PIP_CERT is in os.environ

2 participants

@frenzymadness @gaborbernat