Fix integration tests on Windows #4769

benoit-pierre · 2017-10-05T14:45:12Z

The integration tests can now be run on Windows.

I added Python 3.6 jobs to AppVeyor, however, while I tested with all variants (x86/amd64, 2.7/3.3/3.4/3.5/3.6), this PR only enable the integration tests for the Python27 and Python36-x64 jobs; as with only one job active at a time, enabling integration tests for all jobs will result in a ~3 hours long CI run...

Fix one bug: InstallRequirement.get_dist() would not correctly detect the distribution name (see b327c77).

pradyunsg · 2017-10-05T15:03:45Z

Yayie! Thanks for working on this @benoit-pierre! ^>^

as with only one job active at a time

Can we have multiple jobs run concurrently? I don't know if it's possible but if it is, it'd be nice.

Fix one bug

Which is why we kinda need these tests to be run on Windows. :P

benoit-pierre · 2017-10-05T15:05:18Z

@pradyunsg: https://www.appveyor.com/docs/parallel-testing/#requirements

pradyunsg · 2017-10-05T15:07:44Z

insert sad reaction here

pfmoore · 2017-10-05T15:39:20Z

Yeah, many thanks for this. I'm not sure how easy it's going to be to review this, there's a lot of complexity in here. I'll try to go through it, but as a general comment I will say that the fact that the CI runs are succeeding is a reasonable indication that things look good overall.

I agree with your decision to just run 2.7 and 3.6 x64. The intent here should be to catch Windows-specific issues, not completely re-test everything Travis already covers, so that should be sufficient.

pradyunsg

I gave this a pass and it looks mostly good to me.

pradyunsg · 2017-10-05T16:44:28Z

tests/functional/test_install.py


    # Compute relative install path to FSPkg from scratch path.
    full_rel_path = data.packages.join('FSPkg') - script.scratch_path
+    full_rel_url = ('file:' + full_rel_path +


nit: I would wrap it like:

full_rel_url = ( 'file:' + full_rel_path + '#egg=FSPkg' ).replace(os.path.sep, '/')

pradyunsg · 2017-10-05T16:45:30Z

tests/functional/test_install.py

+    rel_prefix_path = script.scratch / 'prefix'
    install_path = (
-        path / 'site-packages' / 'simple-1.0-py{0}.egg-info'.format(pyversion)
+        distutils.sysconfig.get_python_lib(prefix=rel_prefix_path) /


Not sure how good an idea it is to be depending on/using distutils here and in a few other places; is there some equivalent function in pip's codebase?

We already use it in other places, and in pip's codebase. I don't see an equivalent function we could use.

Thanks for looking! I now think my question was not exactly specific to this PR anyway.

pradyunsg · 2017-10-05T16:46:35Z

tests/functional/test_install.py

    'requiresupper' has install_requires = ['upper']
    """
-    Upper = os.path.join(data.find_links, 'Upper-1.0.tar.gz')
+    Upper = '/'.join((data.find_links, 'Upper-1.0.tar.gz'))


Why not os.path.join?

Because data.find_links is an URL.

Oh. Okay. XD

Maybe better to use urljoin then, just to be explicit? I wouldn't want someone in future to "helpfully" switch back to os.path.join (although the Windows tests would now pick that up 😄)

Except just replacing the example above with urljoin(data.find_links, 'Upper-1.0.tar.gz') does not work... Because:

>>> urljoin('/index/', 'package.tar.gz') '/index/package.tar.gz' >>> urljoin('/index', 'package.tar.gz '/package.tar.gz'

Alternatively, use from posixpath import join as urljoin?

pradyunsg · 2017-10-05T16:56:42Z

@benoit-pierre It would be awesome if you could split out the lsat 2 commits into 2 separate PRs.

They're not really specific to Windows in any way but it'd be nice to handle them independently.

pradyunsg · 2017-10-06T11:44:28Z

@benoit-pierre Could you add a news file for this? :)

IMO it should be a .feature because it's a major change on the development/maintenance side for pip.

dstufft · 2017-10-06T16:27:20Z

Honestly, it probably doesn't need to be mentioned in the change log at all. I don't think users will care that we started running integration tests on Windows (and "how much do users care about X" is sort of the standard to judge if something needs to be in the change log or not).

pradyunsg · 2017-10-06T16:28:56Z

I added one to #4545 on @pfmoore's suggestion. ._.

dstufft · 2017-10-06T16:29:44Z

Lol, well either way is fine :) I personally wouldn't have.

pfmoore · 2017-10-06T16:40:36Z

Ha. My "think of an answer on the spur of the moment" approach has been exposed 😉. I actually like @dstufft's principle of "do users care" here, so I'll reverse myself from before and say I'm OK with not having a news item here.

pfmoore · 2017-10-06T16:43:00Z

.travis/run.sh

Why the change to use --develop here? I don't have a particular problem with it, just don't understand why it's needed (and whether that implies there's an implication of one of the other changes that I've missed).

It's just faster, and we don't need to do a normal installation anymore (since we don't run unvendored tests).

OK, cool, so not specifically related to the Windows changes. That's fine (and faster is always good ;-)).

pfmoore · 2017-10-06T16:49:09Z

Added a couple of minor points, but overall LGTM

pradyunsg · 2017-10-06T16:52:10Z

I'm OK with not having a news item here.

Then I am too. :)

pradyunsg · 2017-10-06T16:59:48Z

@pfmoore So, is this good to go? :3

Match the behavior of `urllib.request.pathname2url`: uppercase the drive letter.

- fix user's bin/scripts directory - support calling `pip` to update pip itself - force IO encoding to UTF-8 to prevent decoding errors - correctly handle the presence '&' characters in arguments to `run`

benoit-pierre · 2017-10-06T17:24:16Z

I moved the last 2 commits to their own PRs, as asked by @pradyunsg.

pradyunsg · 2017-10-06T17:32:55Z

Thank you! ^>^

Once the CI allows, I'll merge them. :)

pfmoore · 2017-10-06T19:27:21Z

@pradyunsg Yep, I'm happy for this to be merged.

pfmoore · 2017-10-06T19:52:20Z

Thanks @benoit-pierre for doing this - it's a huge improvement to our testing on Windows!

pradyunsg · 2017-10-22T16:42:44Z

Linking to #4751.

lock · 2019-06-02T17:05:33Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

pradyunsg added type: maintenance Related to Development and Maintenance Processes OS: windows Windows specific C: tests Testing and related things labels Oct 5, 2017

pradyunsg reviewed Oct 5, 2017

View reviewed changes

pfmoore added the skip news Does not need a NEWS file entry (eg: trivial changes) label Oct 6, 2017

pfmoore reviewed Oct 6, 2017

View reviewed changes

pradyunsg mentioned this pull request Oct 6, 2017

Remove mypy news file #4774

Merged

benoit-pierre added 10 commits October 6, 2017 19:23

fix InstallRequirement.get_dist() on Windows

13cbfd2

tests: skip Bazaar tests if it's not available

3fe6649

tests: skip Mercurial tests if it's not available

18bb9ba

tests: fix TestPipResult.assert_installed on Windows

ba21e15

tests: fix isolate fixture on Windows

6ad82ab

tests: fix virtualenv fixture on Windows

224bbe9

tests: fix path_to_url on Windows

157c6a3

Match the behavior of `urllib.request.pathname2url`: uppercase the drive letter.

tests: fix unit tests on Windows

6421b40

tests: fix PipTestEnvironment implementation on Windows

dd5a5bb

- fix user's bin/scripts directory - support calling `pip` to update pip itself - force IO encoding to UTF-8 to prevent decoding errors - correctly handle the presence '&' characters in arguments to `run`

tests: fix integration tests on Windows

1f6539f

appveyor: enable integration tests and add Python 3.6 jobs

a0a43c1

benoit-pierre force-pushed the win_tests branch from bbd7b17 to a0a43c1 Compare October 6, 2017 17:23

pfmoore merged commit 961737a into pypa:master Oct 6, 2017

benoit-pierre deleted the win_tests branch October 6, 2017 19:52

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019

Uh oh!

Fix integration tests on Windows #4769

Fix integration tests on Windows #4769

Uh oh!

Conversation

benoit-pierre commented Oct 5, 2017

Uh oh!

pradyunsg commented Oct 5, 2017

Uh oh!

benoit-pierre commented Oct 5, 2017

Uh oh!

pradyunsg commented Oct 5, 2017

Uh oh!

pfmoore commented Oct 5, 2017

Uh oh!

pradyunsg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pradyunsg commented Oct 5, 2017

Uh oh!

pradyunsg commented Oct 6, 2017

Uh oh!

dstufft commented Oct 6, 2017

Uh oh!

pradyunsg commented Oct 6, 2017

Uh oh!

dstufft commented Oct 6, 2017

Uh oh!

pfmoore commented Oct 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pfmoore commented Oct 6, 2017

Uh oh!

pradyunsg commented Oct 6, 2017

Uh oh!

pradyunsg commented Oct 6, 2017

Uh oh!

benoit-pierre commented Oct 6, 2017

Uh oh!

pradyunsg commented Oct 6, 2017

Uh oh!

pfmoore commented Oct 6, 2017

Uh oh!

pfmoore commented Oct 6, 2017

Uh oh!

pradyunsg commented Oct 22, 2017

Uh oh!

lock bot commented Jun 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants