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 integration tests for upload with skip_existing #458

Merged
merged 3 commits into from
May 11, 2019

Conversation

bhrutledge
Copy link
Contributor

As a first task for a new contributor at the PyCon 2019 packaging sprint, @brainwane encouraged me to work on #7.

I see opportunities to refactor the tests via constants and fixtures, it seems reasonable to get commands/upload.py to 100% coverage, but I'd like to get feedback on this before proceeding.

Before:

Name                         Stmts   Miss Branch BrPart  Cover
twine/commands/upload.py        48      7     16      5    81%
TOTAL                          748    120    219     38    80%

After:

Name                         Stmts   Miss Branch BrPart  Cover
twine/commands/upload.py        48      3     16      3    91%
TOTAL                          748    116    219     36    81%

@codecov
Copy link

codecov bot commented May 8, 2019

Codecov Report

Merging #458 into master will increase coverage by 0.8%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #458     +/-   ##
=========================================
+ Coverage   79.94%   80.74%   +0.8%     
=========================================
  Files          14       14             
  Lines         748      748             
  Branches      108      108             
=========================================
+ Hits          598      604      +6     
+ Misses        113      110      -3     
+ Partials       37       34      -3
Impacted Files Coverage Δ
twine/wininst.py 31.57% <0%> (ø) ⬆️
twine/commands/register.py 0% <0%> (ø) ⬆️
twine/commands/upload.py 87.5% <0%> (+12.5%) ⬆️

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 c854b2a...e911dd1. Read the comment docs.

Copy link
Member

@theacodes theacodes left a comment

Choose a reason for hiding this comment

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

Thank you for doing this. I left several comments- if you're up to addressing them please do! If not, I'm happy to take this to the finish line for you. Just let me know what you're comfortable with.

@@ -125,6 +125,81 @@ def test_deprecated_repo(tmpdir):
)


def test_upload_prints_skip_message_for_uploaded_package(tmpdir, capsys):
pypirc = os.path.join(str(tmpdir), ".pypirc")
Copy link
Member

Choose a reason for hiding this comment

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

the tmpdir fixture is a py.path.local object, so you can do pypirc_path = tmpdir / ".pypirc".

pypirc = os.path.join(str(tmpdir), ".pypirc")
dists = ["tests/fixtures/twine-1.5.0-py2.py3-none-any.whl"]

with open(pypirc, "w") as fp:
Copy link
Member

Choose a reason for hiding this comment

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

You can use pypirc_path.write_text to simplify this a little

https://py.readthedocs.io/en/latest/path.html#py._path.local.LocalPath.write_text

"""))

upload_settings = settings.Settings(
repository_name="pypi", sign=None, identity=None, username=None,
Copy link
Member

Choose a reason for hiding this comment

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

Style nit: when argument lists end up taking up multiple lines, prefer to make each argument live on its own line:

upload_settings = settings.Settings(
    repository_name="pypi",
    sign=None,
    identity=None,
    ...
)

Also, since most of these are just None, can you omit them?

password:bar
"""))

upload_settings = settings.Settings(
Copy link
Member

Choose a reason for hiding this comment

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

This seems repeated from the last case, can you pull it out into either a factory function or a fixture?

See "Use factory helpers to create complex collaborators" and "Use fixtures sparingly" in https://blog.thea.codes/my-python-testing-style-guide/

@bhrutledge
Copy link
Contributor Author

@theacodes Thanks for the feedback (and the article on testing)! I'm happy to put on my refactoring hat.

bhrutledge added 2 commits May 9, 2019 10:44
- Simplify writing .pypirc
- Remove default Settings
- Reformat lines
- Use WHEEL_FIXTURE
@bhrutledge
Copy link
Contributor Author

@theacodes I've made changes based on your suggestions. I went further with e911dd1, mostly as an exercise with pytest factory fixtures. Happy to edit/revert.

@theacodes theacodes merged commit da07f78 into pypa:master May 11, 2019
@theacodes
Copy link
Member

Thank you, @bhrutledge! :)

@bhrutledge bhrutledge deleted the upload-skip-coverage branch May 11, 2019 11:02
@bhrutledge bhrutledge mentioned this pull request Apr 19, 2020
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.

2 participants