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

Do not add /usr/bin to PATH during tests #298

Conversation

chrahunt
Copy link
Member

@chrahunt chrahunt commented Dec 19, 2019

When scripts (e.g. ansible) already exist in /usr/bin then pipx will
trace warnings that they are already in PATH. This causes test failures
when we assert that no warnings are traced.

The specific test that was failing for me was:

________________________________________ test_install_tricky_packages[ansible] ________________________________________

capsys = <_pytest.capture.CaptureFixture object at 0x7efbf76cd970>, pipx_temp_env = None
caplog = <_pytest.logging.LogCaptureFixture object at 0x7efbf7917c70>, package = 'ansible'

    @pytest.mark.parametrize(
        "package", ["cloudtoken", "awscli", "ansible", "shell-functools"]
    )
    def test_install_tricky_packages(capsys, pipx_temp_env, caplog, package):
        if os.getenv("FAST"):
            pytest.skip("skipping slow tests")

        if sys.platform.startswith("win"):
            pytest.skip("TODO make this work on Windows")
>       install_package(capsys, pipx_temp_env, caplog, package)

tests/test_install.py:57:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

capsys = <_pytest.capture.CaptureFixture object at 0x7efbf76cd970>, pipx_temp_env = None
caplog = <_pytest.logging.LogCaptureFixture object at 0x7efbf7917c70>, package = 'ansible'

    def install_package(capsys, pipx_temp_env, caplog, package):
        run_pipx_cli(["install", package, "--verbose"])
        captured = capsys.readouterr()
        assert f"installed package {package}" in captured.out
        if not sys.platform.startswith("win"):
            # TODO assert on windows too
            # https://github.com/pipxproject/pipx/issues/217
            assert "symlink missing or pointing to unexpected location" not in captured.out
        assert "not modifying" not in captured.out
        assert "is not on your PATH environment variable" not in captured.out
        for record in caplog.records:
>           assert "⚠️" not in record.message
E           AssertionError: assert '⚠️' not in '⚠️  Note: a.../bin/ansible'
E             '⚠️' is contained here:
E               ⚠️  Note: ansible was already on your PATH at /usr/bin/ansible
E             ? ++

tests/test_install.py:39: AssertionError
------------------------------------------------ Captured stdout call -------------------------------------------------

When scripts (e.g. ansible) already exist in /usr/bin then pipx will
trace warnings that they are already in PATH. This causes test failures
when we assert that no warnings are traced.
@chrahunt chrahunt marked this pull request as ready for review December 19, 2019 03:34
@cs01
Copy link
Member

cs01 commented Dec 19, 2019

Thank you for the PR. The reason that workaround is in the test is due to black depending on a package that added gcc as a requirement (pipx installs black as part of its test suite). Other black users are running into the same issue, and are trying to get the gcc dependency eliminated from the black installation process, which may have happened by now. See psf/black#1112.

It might be possible to remove all of the code trying to add gcc to the path in the first place. At the very least, we should add logic such that it only happens on Travis. It might also be the case that if gcc is still required by black, re-adding sudo permissions on Travis will fix the issue (see @itsayellow's comment #266 (comment)).

Before approving and merging these changes, can the following be tried (in this order)? The first solution that passes on travis should be used.

  1. remove the workaround altogether
  2. same as above but also adding in sudo: required to Travis (See remove deprecated keyword "sudo" from travis config #243. I'm not sure if sudo: required has any effect or not anymore?)
  3. only use the workaround if running on travis (this should work). Also add a comment that it should be removed once Installing black 19.10b0 without gcc installed causes an error psf/black#1112 is resolved.
  4. merge this PR with an additional comment that the code should be removed after Installing black 19.10b0 without gcc installed causes an error psf/black#1112 is resolved.

@chrahunt
Copy link
Member Author

#299 (option 1) succeeded, so I'll close this PR in favor of that one.

@chrahunt chrahunt closed this Dec 20, 2019
@chrahunt chrahunt deleted the bugfix/fix-existing-script-test-failures branch December 20, 2019 03:37
@itsayellow
Copy link
Contributor

Tests on my local (macOS) computer have regressed because of this. Now building package regex with clang is causing test fails again on my computer.

@itsayellow
Copy link
Contributor

It looks like this
https://bitbucket.org/mrabarnett/mrab-regex/pull-requests/6/add-bitbucket-pipeline-to-build-linux/diff
May fix it for linux but not for macOS. 🙁

@chrahunt
Copy link
Member Author

We could take a similar approach to this PR:

  1. Have a session-scoped fixture which creates a temporary directory
  2. Populate the directory with symlinks to whitelisted build tools identified by shutil.which like gcc/clang
  3. Depend on that fixture here, adding the temporary directory to PATH

In the future if any other build tools are identified for various platforms then it would just require updating that one workaround fixture.

@itsayellow
Copy link
Contributor

That sounds great. I was just trying to figure out the right way to do that, because I want to test some VCS package specifications that require git in test_install.py .

@itsayellow
Copy link
Contributor

Testing started working for me on macOS. So possibly something got fixed upstream.

@jaraco
Copy link
Member

jaraco commented Jan 31, 2020

Testing started working for me on macOS. So possibly something got fixed upstream.

I had the same issue until I started doing some troubleshooting and ended up causing regex to get compiled to a wheel in my local pip cache. My guess is that if you were to wipe your pip cache, the issue would re-emerge.

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.

4 participants