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

added unittest for _download_git #1544

Merged
merged 3 commits into from Dec 14, 2018

Conversation

4 participants
@kanikas3
Copy link
Contributor

kanikas3 commented Oct 27, 2018

Summary of changes

  • Added unittest for _download_git function

Closes
#1504

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details
@pganssle
Copy link
Member

pganssle left a comment

@kanikas3 and I spoke about the changes that needed to be made at the NYC PyPA sprint, but I've added explicit review comments here just as a reminder and a documentation of what needs to be done.

Overall, the goal of this test should be to make calls into the public interface and verify that they make the correct system calls. To some extent the fact that we're mocking out os.system is testing an implementation detail (since we could easily use subprocess instead), but I think it's a necessary evil if this isn't going to be an implementation test.

Show resolved Hide resolved setuptools/tests/test_packageindex.py Outdated
Show resolved Hide resolved setuptools/tests/test_packageindex.py Outdated
Show resolved Hide resolved setuptools/tests/test_packageindex.py Outdated
@pganssle

This comment has been minimized.

Copy link
Member

pganssle commented Nov 5, 2018

@kanikas3 @vitoace Ping - are you still coming back to this?

@vitoace

This comment has been minimized.

Copy link
Contributor

vitoace commented Nov 5, 2018

Sorry, probably not.

Thanks Paul for the review. Given the waned interest, I've gone ahead and addressed your concerns.

@jaraco

This comment has been minimized.

Copy link
Member

jaraco commented Dec 14, 2018

Thanks @kanikas3 and @vitoace for your contributions here. Although I ended up rewriting some of the changes, your initial contribution saved me a great deal of time placing the test and understanding the basic usage we wished to test. Your commit will be included in the history and I encourage you to look at the subsequent changes for your own edification. Thanks again.

@jaraco jaraco merged commit 361013f into pypa:master Dec 14, 2018

5 checks passed

codecov/patch 100% of diff hit (target 81.45%)
Details
codecov/project 82.28% (+0.82%) compared to 1b98bc2
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

PyPA Sprint Weekend at Bloomberg (2018) automation moved this from Submitted PRs to Merged PRs Dec 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment