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

Test against Python 3.3,3.4,3.5,3.6 #34

Closed
wants to merge 3 commits into from

Conversation

Flimm
Copy link

@Flimm Flimm commented Jan 19, 2017

The tests currently fail because Python 2.6 has reached end-of-life. So let's stop testing against Python 2.6.

Since this supports Python 3.x, test against it too.

Flimm added 3 commits January 19, 2017 17:19
Python 2.6 is end-of-life and it has started giving this deprecation
warning, causing tests to fail:

    DEPRECATION: Python 2.6 is no longer supported by the Python core
    team, please upgrade your Python. A future version of pip will drop
    support for Python 2.6
Changes required:

- Omit deprecated --use-mirrors argument for pip during testing.
- Ignore exit code of know bad pip command.
- A test checking older pip versions is skipped for Python 3.6.
- "pip install cram --use-mirrors"
- "pip install . --use-mirrors"
- "pip install cram"
- "pip install ."
Copy link
Owner

Choose a reason for hiding this comment

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

This is an OK change. --use-mirrors has been removed as of pip 7.0 and it isn't really needed. Would be nice if you could submit this change separately.

Copy link
Author

Choose a reason for hiding this comment

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

OK, I've created the pull request. #35

I've had multiple tiny pull requests be rejected in the past because the contributors don't want to have to merge or rebase the changes, so that's why I did it this way.

> else
> echo Skipped
> fi
(Everything up-to-date|Skipped) (re)

Copy link
Owner

Choose a reason for hiding this comment

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

This is a useful bit. pip<6.0 won't install with the newest versions of Python, so skipping this test is justified in that case. It doesn't hurt to have this in the tests even when we aren't testing Python 3 yet, so I would happily accept this bit in a separate pull request.

Copy link
Author

Choose a reason for hiding this comment

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

Separate pull request: #36

@jgonggrijp
Copy link
Owner

While I value your willingness to contribute, unfortunately I cannot accept this pull request (at least not as a whole). There are two main reasons.

Firstly, there is a purpose for cElementTree being in the tests: the comments mention that once upon a time, it caused a warning message. It is a regression test, put there specifically to ensure that some particular bug doesn't turn up again. You can't just ignore cElementTree under Python 3 because it doesn't install, because then we won't know it if the bug returns under Python 3. The only proper way to fix the test for Python 3, is to research why cElementTree was added in the first place and then decide what to do next. We may find either that the regression test is not relevant anymore, in which case we can remove it entirely, or that the test is still relevant, in which case we need to find a replacement for cElementTree which does install under Python 3. See also 9c6a53a. (If this seems overly conservative to you, please read this).

Secondly, pip-review is a remnant of an old version of the pip-tools package. Many of its current users probably know it from that era, when Python 2.6 was still fully alive. Some of those users might still be stuck with Python 2.6 for other reasons than laziness (see e.g. #2). Right now, pip-review still works under Python 2.6 without requiring any additional effort. As long as this is the case, I want to see it from the tests, so those users who are stuck in the past aren't needlessly left in the dark. I also explained this in #15. Concluding, it is too early to remove Python 2.6 from the tests. The problem with Python 2.6 should instead be solved by adding an optional line to the test so that the pip warning message about Python 2.6 isn't unexpected anymore.

I do think that some of the changes you propose would be useful. I would be grateful if you could submit those changes in a separate, smaller pull request. Please see my comments in the "Files changed" tab for some details.

@jgonggrijp jgonggrijp closed this Jan 19, 2017
@@ -16,7 +16,7 @@ Setup. Let's pretend we have some outdated package versions installed:

Also install library, which caused warning message:

$ pip install http://www.effbot.org/media/downloads/cElementTree-1.0.5-20051216.tar.gz >/dev/null 2>&1
$ pip install http://www.effbot.org/media/downloads/cElementTree-1.0.5-20051216.tar.gz >/dev/null 2>&1 || true
Copy link
Owner

Choose a reason for hiding this comment

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

Your commit message claims to "Ignore exit code of know bad pip command" (sic). But pip install http://valid.domain.xyz/valid/path/ending/with.tar.gz is not a "known bad pip command". To the contrary! This command is perfectly valid and it is in the tests for a reason. If it fails you can't just ignore that; you have to do something about it.

@Flimm
Copy link
Author

Flimm commented Jan 20, 2017

You're right, I didn't put much thought into the cElementTree test. I've now looked into it more, and it seems to come from this commit: 243cc0c

This commit was inherited from pip-tools, and it seemed to have been trying to fix this issue: jazzband#9

However, I can't get pip freeze to print out that comment even under Python 2.6 and pip 1.5.6.

Interestingly, review.t used to contain these lines:

Warning: cannot find svn location for cElementTree==1.0.5-20051216
cElementTree==* is available (you have 1.0.5-20051216) (glob)

However, this commit changed that: fe42015

So it seems that this test is no longer testing what is supposed to be testing anyway, under Python 2 or Python 3.

That's as far as I've got time to investigate, but I think tests should be enabled for Python 3.x in any case.


As for supported an unsupported legacy Python version (2.6), that's your choice.

@jgonggrijp
Copy link
Owner

Thank you for the valuable research. I'll look into it further.

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