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 test for installing pysnooper #76

Closed
wants to merge 5 commits into from
Closed

Add integration test for installing pysnooper #76

wants to merge 5 commits into from

Conversation

angrydead
Copy link

#39: Add tox environment for installing pysnooper.

@cool-RR cool-RR requested a review from bittner April 29, 2019 17:32
@cool-RR
Copy link
Owner

cool-RR commented Apr 29, 2019

Cool, can you please add a line that at least imports pysnooper to ensure that it's installed?

@angrydead
Copy link
Author

Cool, can you please add a line that at least imports pysnooper to ensure that it's installed?

Yes, I will do that with pip show pysnooper, which will verify the package name and version.

@alexmojaki
Copy link
Collaborator

You need a line with TOXENV=pysnooper in travis.yml. Don't know what the right stage would be, I guess test?

@alexmojaki
Copy link
Collaborator

I don't know why but the logs don't show the desired commands being run: https://travis-ci.org/cool-RR/PySnooper/jobs/526204104

[pysnooper]
commands =
pip install pysnooper
pip show pysnooper
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is unneeded. You can remove it. (Explanation: The [<tool_name>] sections are configuration file sections that the tool reads. I don't think pysnooper reads anything out from tox.ini.)

@@ -54,6 +56,13 @@ deps = pip-tools
commands = pip-compile --output-file requirements.txt requirements.in
changedir = {toxinidir}

[testenv:pysnooper]
description = Ensure pip installs pysnooper
deps = pip
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to install pip. Replace this by pysnooper if you want to install it off PyPI (that's probably not what you want), or leave it empty.

deps = pip
commands =
pip install pysnooper
pip show pysnooper
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not an integration test.

Think: What does this verify? Is the pysnooper code run with it? (It's not.)

Also, installing pysnooper is unneeded, and even if it were you can move it to deps = .

@@ -16,6 +17,7 @@ description = Unit tests
deps =
pytest
python_toolbox
pysnooper
Copy link
Collaborator

Choose a reason for hiding this comment

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

This installs the latest released version of pysnooper off PyPI. This is most probably not what you want.

Note that pysnooper (off the checked out commit) is already installed by Tox automatically. Try running pysnooper or python -c "import pysnooper; print(pysnooper.__version__)" to prove it.

- { stage: test, python: 2.7, env: TOKENV=pysnooper }
- { stage: test, python: 3.8-dev, env: TOKENV=pysnooper }
- { stage: test, python: pypy2.7-6.0, env: TOKENV=pysnooper }
- { stage: test, python: pypy3.5, env: TOKENV=pysnooper }
Copy link
Collaborator

@bittner bittner Apr 30, 2019

Choose a reason for hiding this comment

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

This looks very verbose.

What if you simply would run your integration test with the [testenv] section? (Then all this would not be needed. Hmm?)

@bittner
Copy link
Collaborator

bittner commented Apr 30, 2019

I think, this contribution can be much simpler:

Simply add an additional command that performs the desired integration test to the [testenv]'s commands attribute, e.g.

commands =
    pytest
    python -c "print('run my integration test')"

I'd also suggest to add --verbose as a pytest switch (as long as you don't have hundreds of tests), e.g.

[pytest]
addopts = --strict --verbose

This makes the output easier to understand. I see you removed it, Ram, due to problems with the IDE, hmm.

@bittner
Copy link
Collaborator

bittner commented Apr 30, 2019

Please note, everyone, that pysnooper is always installed by Tox automatically. That's the whole purpose of a Tox setup: It installs the package under test (using setup.py, which is always done unless skipsdist = true).

Take a look at older builds to prove yourself. Note the zip (created by setuptools) and the installed pysnooper version (even though it's not specified anyhere explicitly for installation).

@angrydead
Copy link
Author

Thanks for your input @bittner. I'll make these changes as soon as I can.

@angrydead
Copy link
Author

Actually, I'm just going to close out this PR.

@angrydead angrydead closed this Apr 30, 2019
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.

None yet

4 participants