Repository: pipxproject/pipx
Pull request #453 (2020-07-26 22:31:49)

Disable animation if terminal width is reported as 0 or small
<!-- add an 'x' in the brackets below -->
[x] I have added an entry to `docs/changelog.md`

## Summary of changes
Fixes bug #444 .

Some terminal / interactive programs and mixtures cause the terminal columns to be reported by `shutil.get_terminal_size` as 0.  I was able to find some examples online talking about the interactive shell inside of emacs doing this sometimes.  

In general, if the terminal columns are smaller than our continuation characters, the animate thread will crash with a python stack trace, and this is unacceptable.

This PR prevents the thread crash from happening.

## Test plan
<!-- provide evidence of testing, preferably with command(s) that can be copy+pasted by others -->
Tested by running
```
env COLUMNS=2 pipx install pycowsay
```
For current pipx the animate thread crashes with a `ValueError` and a stacktrace.  For pipx with this PR patch it executes successfully.


itsayellow: Is this worth a separate test in `test_animate.py`?
itsayellow: I added a print statement so that even if we do not do animation, we still print `message` with an ellipsis tacked on once to stdout to indicate something is happening.  (e.g. "creating virtual environment...", "installing pycowsay...")

Let me know if people like that idea.
itsayellow: I also modified `test_shared_libs.py` because one of the macos tests took 22 minutes and broke the test.  The problem is that `now` is computed at import, and if the test for "not time to update shared libraries" is set only 20 minutes before the deadline, by the end of the test it unfortunately becomes "time to update shared libraries".

To fix the wide variances of time between import and test, I moved the setting of `now` inside the test function.
itsayellow: Additionally I revamped the animation tests to use parametrization instead of for-loops.  This is easier to read and gives better test isolation.  It eliminates past problems with stderr from one sub-test affecting the next sub-test.

I also added a test to verify that `stderr_is_tty=False` or any term_cols less than 17 will disable animation.

Patch:
diff --git a/docs/changelog.md b/docs/changelog.md
index 1ca0126..8055cfe 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -1,4 +1,5 @@
 dev
+- Disabled animation when we cannot determine terminal size or if the number of columns is too small. (Fixes #444)
 - Version of each injected package is now listed after name for `pipx list --include-injected`
 - Change metadata recorded from version-specified install to allow upgrades in future.  Adds pipx dependency on `packaging` package.
 - [bugfix] Prevent python error in case where package has no pipx metadata and advise user how to fix.
diff --git a/docs/comparisons.md b/docs/comparisons.md
index 6b9a1bf..203f3b1 100644
--- a/docs/comparisons.md
+++ b/docs/comparisons.md
@@ -31,7 +31,7 @@ pipx installs packages to environments created with venv. `pipx install black --
 * pipx installs packages in virtual environments and exposes their entrypoints on your PATH
 
 Example interaction:
-Install a Python interpreter with pyenv, then install a package using pipx and that new interpreter: `pipx install black --python3.7` where python3.7 was installed on the system with pyenv
+Install a Python interpreter with pyenv, then install a package using pipx and that new interpreter: `pipx install black --python=python3.7` where python3.7 was installed on the system with pyenv
 
 ## pipx vs pipsi
 * pipx and pipsi both install packages in a similar way
@@ -85,4 +85,3 @@ pip-run poetry -- -m poetry --help
 
 Example interaction:
 None.
-
diff --git a/docs/docs.md b/docs/docs.md
index 4821bbd..0c61e27 100644
--- a/docs/docs.md
+++ b/docs/docs.md
@@ -48,10 +48,11 @@ subcommands:
                         directory (experimental).
     runpip              Run pip in an existing pipx-managed Virtual
                         Environment
-    ensurepath          Ensure directory where pipx stores apps is on your
-                        PATH environment variable. Note that running this may
-                        modify your shell's configuration file(s) such as
-                        '~/.bashrc'.
+    ensurepath          Ensure directory where pipx stores apps is in your
+                        PATH environment variable. Also if pipx was installed
+                        via `pip install --user`, ensure pipx itself is in
+                        your PATH. Note that running this may modify your
+                        shell's configuration file(s) such as '~/.bashrc'.
     completions         Print instructions on enabling shell completions for
                         pipx
 
diff --git a/noxfile.py b/noxfile.py
index fb6c14f..d9a43d3 100644
--- a/noxfile.py
+++ b/noxfile.py
@@ -1,11 +1,9 @@
-import os
 import subprocess
 import sys
 from pathlib import Path
 
 import nox  # type: ignore
 
-
 # NOTE: these tests require nox to create virtual environments
 # with venv. nox currently uses virtualenv. pipx
 # uses a fork of nox at https://github.com/cs01/nox
@@ -15,12 +13,7 @@ import nox  # type: ignore
 # until this is fixed in nox. See
 # https://github.com/theacodes/nox/issues/199
 
-
-travis_python_version = os.environ.get("TRAVIS_PYTHON_VERSION")
-if travis_python_version:
-    python = [travis_python_version]
-else:
-    python = ["3.6", "3.7", "3.8"]
+python = ["3.6", "3.7", "3.8"]
 
 if sys.platform == "win32":
     # docs fail on Windows, even if `chcp.com 65001` is used
diff --git a/src/pipx/animate.py b/src/pipx/animate.py
index 2b1567c..83804e1 100644
--- a/src/pipx/animate.py
+++ b/src/pipx/animate.py
@@ -16,6 +16,12 @@ EMOJI_ANIMATION_FRAMES = ["⣷", "⣯", "⣟", "⡿", "⢿", "⣻", "⣽", "⣾"
 NONEMOJI_ANIMATION_FRAMES = ["", ".", "..", "..."]
 EMOJI_FRAME_PERIOD = 0.1
 NONEMOJI_FRAME_PERIOD = 1
+MINIMUM_COLS_ALLOW_ANIMATION = 16
+
+
+def _env_supports_animation():
+    (term_cols, _) = shutil.get_terminal_size(fallback=(0, 0))
+    return stderr_is_tty and term_cols > MINIMUM_COLS_ALLOW_ANIMATION
 
 
 @contextmanager
@@ -23,8 +29,9 @@ def animate(
     message: str, do_animation: bool, *, delay: float = 0
 ) -> Generator[None, None, None]:
 
-    if not do_animation or not stderr_is_tty:
-        # no op
+    if not do_animation or not _env_supports_animation():
+        # No animation, just a single print of message
+        print(f"{message}...")
         yield
         return
 


Test patch:
diff --git a/tests/test_animate.py b/tests/test_animate.py
index 9176a49..cad55c9 100644
--- a/tests/test_animate.py
+++ b/tests/test_animate.py
@@ -1,14 +1,19 @@
 import time
 
+import pytest  # type: ignore
+
 import pipx.animate
 from pipx.animate import (
     CLEAR_LINE,
     EMOJI_ANIMATION_FRAMES,
-    NONEMOJI_ANIMATION_FRAMES,
     EMOJI_FRAME_PERIOD,
+    NONEMOJI_ANIMATION_FRAMES,
     NONEMOJI_FRAME_PERIOD,
 )
 
+# 40-char test_string counts columns e.g.: "0204060810 ... 363840"
+TEST_STRING_40_CHAR = "".join([f"{x:02}" for x in range(2, 41, 2)])
+
 
 def check_animate_output(
     capsys,
@@ -19,8 +24,9 @@ def check_animate_output(
     extra_animate_time=0.4,
     extra_after_thread_time=0.1,
 ):
-    # NOTE: extra_animate_time <= 0.3 failed on macos
-    #       extra_after_thread_time <= 0.0 failed on macos
+    # github workflow history (2020-07-27):
+    #     extra_animate_time <= 0.3 failed on macos
+    #     extra_after_thread_time <= 0.0 failed on macos
     expected_string = "".join(frame_strings)
 
     chars_to_test = 1 + len("".join(frame_strings[:frames_to_test]))
@@ -28,8 +34,7 @@ def check_animate_output(
     with pipx.animate.animate(test_string, do_animation=True):
         time.sleep(frame_period * (frames_to_test - 1) + extra_animate_time)
     # Wait before capturing stderr to ensure animate thread is finished
-    #   and to capture all its characters. If some are left over they can cause
-    #   false fail in the next call of check_animate_output()
+    #   and to capture all its characters.
     time.sleep(extra_after_thread_time)
     captured = capsys.readouterr()
 
@@ -38,11 +43,6 @@ def check_animate_output(
         print(
             "Not enough captured characters--Likely need to increase extra_animate_time"
         )
-    if (
-        captured.err[: len(frame_strings[0])]
-        != expected_string[: len(frame_strings[0])]
-    ):
-        print("Error in first frame--Might need to increase extra_after_thread_time")
     print(f"captured characters: {len(captured.err)}")
     print(f"chars_to_test: {chars_to_test}")
     for i in range(0, chars_to_test, 40):
@@ -64,52 +64,79 @@ def test_delay_suppresses_output(capsys, monkeypatch):
     assert test_string not in captured.err
 
 
-def test_line_lengths_emoji(capsys, monkeypatch):
+@pytest.mark.parametrize(
+    "env_columns,expected_frame_message",
+    [
+        (45, f"{TEST_STRING_40_CHAR:.{45-6}}..."),
+        (46, f"{TEST_STRING_40_CHAR}"),
+        (47, f"{TEST_STRING_40_CHAR}"),
+    ],
+)
+def test_line_lengths_emoji(capsys, monkeypatch, env_columns, expected_frame_message):
     # emoji_support and stderr_is_tty is set only at import animate.py
     # since we are already after that, we must override both here
     monkeypatch.setattr(pipx.animate, "stderr_is_tty", True)
     monkeypatch.setattr(pipx.animate, "emoji_support", True)
 
-    frames_to_test = 4
-
-    # 40-char test_string counts columns e.g.: "0204060810 ... 363840"
-    test_string = "".join([f"{x:02}" for x in range(2, 41, 2)])
-
-    columns_to_test = [45, 46, 47]
-    expected_message = [f"{test_string:.{45-6}}...", f"{test_string}", f"{test_string}"]
-
-    for i, columns in enumerate(columns_to_test):
-        monkeypatch.setenv("COLUMNS", str(columns))
-
-        frame_strings = [
-            f"{CLEAR_LINE}\r{x} {expected_message[i]}" for x in EMOJI_ANIMATION_FRAMES
-        ]
-        check_animate_output(
-            capsys, test_string, frame_strings, EMOJI_FRAME_PERIOD, frames_to_test
-        )
-
+    monkeypatch.setenv("COLUMNS", str(env_columns))
 
-def test_line_lengths_no_emoji(capsys, monkeypatch):
+    frames_to_test = 4
+    frame_strings = [
+        f"{CLEAR_LINE}\r{x} {expected_frame_message}" for x in EMOJI_ANIMATION_FRAMES
+    ]
+    check_animate_output(
+        capsys, TEST_STRING_40_CHAR, frame_strings, EMOJI_FRAME_PERIOD, frames_to_test
+    )
+
+
+@pytest.mark.parametrize(
+    "env_columns,expected_frame_message",
+    [
+        (43, f"{TEST_STRING_40_CHAR:.{43-4}}"),
+        (44, f"{TEST_STRING_40_CHAR}"),
+        (45, f"{TEST_STRING_40_CHAR}"),
+    ],
+)
+def test_line_lengths_no_emoji(
+    capsys, monkeypatch, env_columns, expected_frame_message
+):
     # emoji_support and stderr_is_tty is set only at import animate.py
     # since we are already after that, we must override both here
     monkeypatch.setattr(pipx.animate, "stderr_is_tty", True)
     monkeypatch.setattr(pipx.animate, "emoji_support", False)
 
+    monkeypatch.setenv("COLUMNS", str(env_columns))
+
     frames_to_test = 2
+    frame_strings = [
+        f"{CLEAR_LINE}\r{expected_frame_message}{x}" for x in NONEMOJI_ANIMATION_FRAMES
+    ]
 
-    # 40-char test_string counts columns e.g.: "0204060810 ... 363840"
-    test_string = "".join([f"{x:02}" for x in range(2, 41, 2)])
+    check_animate_output(
+        capsys,
+        TEST_STRING_40_CHAR,
+        frame_strings,
+        NONEMOJI_FRAME_PERIOD,
+        frames_to_test,
+    )
 
-    columns_to_test = [43, 44, 45]
-    expected_message = [f"{test_string:.{43-4}}", f"{test_string}", f"{test_string}"]
 
-    for i, columns in enumerate(columns_to_test):
-        monkeypatch.setenv("COLUMNS", str(columns))
+@pytest.mark.parametrize(
+    "env_columns,stderr_is_tty", [(0, True), (8, True), (16, True), (17, False)]
+)
+def test_env_no_animate(capsys, monkeypatch, env_columns, stderr_is_tty):
+    monkeypatch.setattr(pipx.animate, "stderr_is_tty", stderr_is_tty)
+    monkeypatch.setenv("COLUMNS", str(env_columns))
 
-        frame_strings = [
-            f"{CLEAR_LINE}\r{expected_message[i]}{x}" for x in NONEMOJI_ANIMATION_FRAMES
-        ]
+    frames_to_test = 4
+    expected_string = f"{TEST_STRING_40_CHAR}...\n"
+    extra_animate_time = 0.4
+    extra_after_thread_time = 0.1
 
-        check_animate_output(
-            capsys, test_string, frame_strings, NONEMOJI_FRAME_PERIOD, frames_to_test
-        )
+    with pipx.animate.animate(TEST_STRING_40_CHAR, do_animation=True):
+        time.sleep(EMOJI_FRAME_PERIOD * (frames_to_test - 1) + extra_animate_time)
+    time.sleep(extra_after_thread_time)
+    captured = capsys.readouterr()
+
+    assert captured.out == expected_string
+    assert captured.err == ""
diff --git a/tests/test_shared_libs.py b/tests/test_shared_libs.py
index 5feeddb..b545d7c 100644
--- a/tests/test_shared_libs.py
+++ b/tests/test_shared_libs.py
@@ -4,21 +4,19 @@ import pytest  # type: ignore
 from pipx import shared_libs
 
 
-now = time.time()
-
-
 @pytest.mark.parametrize(
-    "mtime,needs_upgrade",
+    "mtime_minus_now,needs_upgrade",
     [
-        (now - shared_libs.SHARED_LIBS_MAX_AGE_SEC - 20 * 60, True),
-        (now - shared_libs.SHARED_LIBS_MAX_AGE_SEC + 20 * 60, False),
+        (-shared_libs.SHARED_LIBS_MAX_AGE_SEC - 5 * 60, True),
+        (-shared_libs.SHARED_LIBS_MAX_AGE_SEC + 5 * 60, False),
     ],
 )
-def test_auto_update_shared_libs(capsys, pipx_temp_env, mtime, needs_upgrade):
+def test_auto_update_shared_libs(capsys, pipx_temp_env, mtime_minus_now, needs_upgrade):
+    now = time.time()
     shared_libs.shared_libs.create([], verbose=True)
     shared_libs.shared_libs.has_been_updated_this_run = False
 
     access_time = now  # this can be anything
-    os.utime(shared_libs.shared_libs.pip_path, (access_time, mtime))
+    os.utime(shared_libs.shared_libs.pip_path, (access_time, mtime_minus_now + now))
 
     assert shared_libs.shared_libs.needs_upgrade is needs_upgrade