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

Install fresh virtualenv on AppVeyor #984

Merged
merged 2 commits into from
Nov 10, 2019

Conversation

atugushev
Copy link
Member

@atugushev atugushev commented Nov 9, 2019

Fixes #983. See #983 (comment).

@atugushev atugushev added bug fix tests Testing and related things labels Nov 9, 2019
@codecov
Copy link

codecov bot commented Nov 9, 2019

Codecov Report

Merging #984 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #984   +/-   ##
======================================
  Coverage    99.1%   99.1%           
======================================
  Files          34      34           
  Lines        2350    2350           
  Branches      305     305           
======================================
  Hits         2329    2329           
  Misses         11      11           
  Partials       10      10

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 69e4142...8514a97. Read the comment docs.

Copy link
Member

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

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

I have but one tiny concern, technically it's fine though.

.appveyor.yml Outdated
@@ -102,6 +102,7 @@ matrix:

install:
- "SET PATH=%PYTHON%;%PYTHON%\\Scripts;%PATH%"
- pip install -U virtualenv
Copy link
Member

Choose a reason for hiding this comment

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

Why not install virtualenv and tox within one line?
I'd prefer you use --update, since it's more human readable.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a workaround and easy to git-blame. Plus tox doesn't need to be upgraded.

Maybe we should better put there a comment about why do we really need to upgrade virtualenv?

Copy link
Member Author

@atugushev atugushev Nov 10, 2019

Choose a reason for hiding this comment

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

Actual, I prefer long-options too, especially in tests when we run something like invoke(cli, ["--upgrade-package", "foo"]), cause I always forget what does -P mean.

These pip -e -r -U options are so famous and I type them because of a muscle memory 😅 We could normalize all options here and in the other configs in a separate PR. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, we can remove this workaround after tox releases a new version, where virtualenv has bumped to >=16.0.0.

@atugushev atugushev merged commit 58a3eb1 into jazzband:master Nov 10, 2019
@atugushev atugushev deleted the fix-issue-983 branch November 10, 2019 07:07
@atugushev
Copy link
Member Author

@codingjoe thanks for reviewing this!

@atugushev atugushev mentioned this pull request May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Testing and related things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_realistic_complex_sub_dependencies fails on Windows py27-pip8.1.1
2 participants