Skip to content

added unittest for _download_git#1544

Merged
jaraco merged 3 commits into
pypa:masterfrom
kanikas3:added-unittest-download-git
Dec 14, 2018
Merged

added unittest for _download_git#1544
jaraco merged 3 commits into
pypa:masterfrom
kanikas3:added-unittest-download-git

Conversation

@kanikas3

@kanikas3 kanikas3 commented Oct 27, 2018

Copy link
Copy Markdown
Contributor

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 pganssle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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.

Comment thread setuptools/tests/test_packageindex.py Outdated
Comment thread setuptools/tests/test_packageindex.py Outdated
Comment thread setuptools/tests/test_packageindex.py Outdated
@pganssle

pganssle commented Nov 5, 2018

Copy link
Copy Markdown
Member

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

@vitoace

vitoace commented Nov 5, 2018

Copy link
Copy Markdown
Contributor

Sorry, probably not.

@jaraco jaraco dismissed pganssle’s stale review December 14, 2018 20:12

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

@jaraco

jaraco commented Dec 14, 2018

Copy link
Copy Markdown
Member

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
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.

4 participants