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

Add a test for uncovered branch in the writer module #1101

Merged
merged 1 commit into from
Apr 10, 2020

Conversation

atugushev
Copy link
Member

@atugushev atugushev commented Apr 2, 2020

There was uncovered unnecessary branch. Now it's 100% coverage in the writer module.

@atugushev atugushev added refactor Refactoring code tests Testing and related things writer Related to results output writer component labels Apr 2, 2020
@codecov
Copy link

codecov bot commented Apr 2, 2020

Codecov Report

Merging #1101 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1101   +/-   ##
=======================================
  Coverage   99.42%   99.43%           
=======================================
  Files          34       34           
  Lines        2441     2458   +17     
  Branches      302      302           
=======================================
+ Hits         2427     2444   +17     
  Misses          8        8           
  Partials        6        6           
Impacted Files Coverage Δ
tests/test_writer.py 100.00% <100.00%> (ø)
piptools/repositories/pypi.py 93.14% <0.00%> (-0.58%) ⬇️
piptools/resolver.py 100.00% <0.00%> (ø)
tests/test_cli_compile.py 100.00% <0.00%> (ø)
piptools/writer.py 100.00% <0.00%> (+0.74%) ⬆️

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 878fb31...ce125e0. Read the comment docs.

Copy link
Contributor

@AndydeCleyre AndydeCleyre left a comment

Choose a reason for hiding this comment

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

I had a look to understand the cases covered or not, and figured out that this change would result in a false-positive warning in the case that:

  • --generate-hashes is passed, and
  • only editable requirements are present

The editable requirements don't (can't) get hashed, and if there were also hashed reqs then pip would refuse to cooperate, but since none of them are, the results are no different from not passing --generate-hashes, so pip will have no problem.

It may be the case that a warning is still appropriate, if unnecessary, since hashes were requested, but the current language says "pip install will require the following package to be hashed," which isn't true in this case.

Reproduction (using plumbum since it has no deps):

$ git clone git@github.com:tomerfiliba/plumbum
$ cd plumbum
$ echo '-e .' > requirements.in
$ pip-compile --no-header --generate-hashes requirements.in
# WARNING: pip install will require the following package to be hashed.
# Consider using a hashable URL like https://github.com/jazzband/pip-tools/archive/SOMECOMMIT.zip
-e file:///home/andy/Code/plumbum  # via -r requirements.in
The generated requirements file may be rejected by pip install. See # WARNING lines for details.

@atugushev
Copy link
Member Author

@AndydeCleyre awesome catch! 🎉 That's so tricky. I'll add a test for that case then.

@AndydeCleyre
Copy link
Contributor

AndydeCleyre commented Apr 4, 2020

The test looks good 👍 . Is it still the plan to change the has_hashes tracking at all, or is this PR just for the test now?

EDIT: If it's the latter, please just update the PR title.

@atugushev atugushev changed the title Remove unnecessary has_hashes variable Add a test for uncovered branch in the writer module Apr 10, 2020
@atugushev
Copy link
Member Author

@AndydeCleyre

Title updated.

Copy link
Contributor

@AndydeCleyre AndydeCleyre left a comment

Choose a reason for hiding this comment

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

Perfect, thanks!

@atugushev atugushev merged commit 1c503c9 into jazzband:master Apr 10, 2020
@atugushev atugushev deleted the writer-coverage branch April 10, 2020 14:50
@atugushev
Copy link
Member Author

@AndydeCleyre

Thank you for the analysis and for reviewing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring code tests Testing and related things writer Related to results output writer component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants