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

fix(tests): explicitly state the shell to use #844

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zappolowski
Copy link
Contributor

tmux uses the default login shell and thus tests might fail if this is not bash (or something compatible). Explicitly stating which shell to use circumvents issues arising from accidentally using another shell.

@codecov
Copy link

codecov bot commented Nov 2, 2022

Codecov Report

Merging #844 (0b129c6) into master (d63f9f5) will not change coverage.
The diff coverage is n/a.

❗ Current head 0b129c6 differs from pull request most recent head 727ab39. Consider uploading reports for the commit 727ab39 to get more accurate results

@@           Coverage Diff           @@
##           master     #844   +/-   ##
=======================================
  Coverage   76.46%   76.46%           
=======================================
  Files          24       24           
  Lines        1606     1606           
  Branches      362      362           
=======================================
  Hits         1228     1228           
  Misses        269      269           
  Partials      109      109           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tony
Copy link
Member

tony commented Nov 3, 2022

tmux uses the default login shell and thus tests might fail if this is not bash (or something compatible). Explicitly stating which shell to use circumvents issues arising from accidentally using another shell.

Perhaps it may be beneficial or necessary to set a default-shell in tests.

Out of curiousity, without this, what error do you get? What distro / system are you running into an issue on?

As for /bin/bash, it isn't available in a default OpenBSD / FreeBSD installation, for instance. It may not be the most portable solution.

@zappolowski
Copy link
Contributor Author

zappolowski commented Nov 3, 2022

Out of curiousity, without this, what error do you get? What distro / system are you running into an issue on?

I'm on Arch, but I think the issue is my (login) shell: /usr/bin/fish ... as using /bin/bash avoids the issue.

Running (on current master):
py.test tests/workspace/test_builder.py -k 'test_load_workspace_enter[pane_enter_default_shortform]' > pytest.log
yields pytest.log

There are two issues with fish for me:

  • it doesn't support the $((nummeric_stuff)) syntax used in the test (I'm not sure which shells support this, but fish and e.g. tcsh don't)
  • fish prints out a welcome message, which interferes with the capturing (as far as I can see there's already fixture code for suppressing something similar for zsh)

As for /bin/bash, it isn't available in a default OpenBSD / FreeBSD installation, for instance. It may not be the most portable solution.

I think, /bin/sh could be worth a try, though it might also end up using a shell which doesn't support everything currently used in the tests.

@tony
Copy link
Member

tony commented Nov 4, 2022

Thank you!

I will look closer during the weekend. I'd like to get arch / perhaps freebsd setup locally

Perhaps default-shell: /bin/bash is the best approach. Another option would be to have a CI job that sets up arch and a few other distros to run pytest.

@tony
Copy link
Member

tony commented Nov 5, 2022

@zappolowski On a fresh Arch installation, I initially got a similar error. I don't have fish. But I think it's due to not having tmux(1) installed yet.

It started working after installing a few packages / configurations. Likely since the arch package for tmuxp installs tmux.

$ sudo pacman -Qe
arch-install-scripts 27-1
base 3-1
dhcpcd 9.4.1-1
diffutils 3.8-1
git 2.38.1-2
groff 1.22.4-7
inetutils 2.3-1
keychain 2.8.5-2
logrotate 3.20.1-1
make 4.3-3
man-db 2.11.0-1
man-pages 6.01-1
nano 6.4-1
netctl 1.28-1
nodejs 19.0.1-1
openresolv 3.12.0-1
openssh 9.1p1-3
python 3.10.8-3
sudo 1.9.12-5
sysfsutils 2.1.1-1
texinfo 6.8-2
vcspull 1.18.0-1
vi 1:070224-6
vim 9.0.0814-1
which 2.21-5
yarn 1.22.19-1

I created a second user: py.test works fine also.

@zappolowski
Copy link
Contributor Author

zappolowski commented Nov 5, 2022

As I've said the culprit is not Arch, but my login-shell, which is fish, which is (intentionally) not POSIX-compatible.

I've created two (hopefully) portable containers to verify this (TBH it was a bit of a PITA to figure out, why the tests failed as man pages were missing):
build_tmuxp_arch_fish_image.sh.txt
build_tmuxp_arch_bash_image.sh.txt

If you have buildah and podman installed, you can just build the images by executing the script and then run them by

$ podman run --rm --volume $(pwd):/src:ro localhost/arch-bash-tmuxp-dev:latest
$ podman run --rm --volume $(pwd):/src:ro localhost/arch-fish-tmuxp-dev:latest

The first one should succeed, the second one should fail.

Edit: Using the bash image I can locally run the test-suite without issues, which would be okay for me. It's up to you to decide whether it"s worth the effort of supporting "special" configurations like mine.

Edit2: A note on running the containers. As /src is mounted read-only you have to set in-project = false in poetry.toml.

@tony
Copy link
Member

tony commented Nov 5, 2022

This is the error I got with fish(1) above

podman run --rm --interactive --tty --volume $(pwd):/src:rw localhost/arch-fish-tmuxp-dev:latest
Installing the current project: libtmux (0.15.9)
============================================================================================================ test session starts ============================================================================================================collected 180 items

src/libtmux/pane.py .RRF                                                                                                                                                                                                              [  1%]
src/libtmux/pytest_plugin.py ..                                                                                                                                                                                                       [  2%]
src/libtmux/server.py ..                                                                                                                                                                                                              [  3%]
src/libtmux/session.py .                                                                                                                                                                                                              [  3%]
src/libtmux/test.py ......                                                                                                                                                                                                            [  7%]
src/libtmux/window.py ..                                                                                                                                                                                                              [  8%]
tests/test_common.py ....................                                                                                                                                                                                             [ 19%]
tests/test_pane.py .....                                                                                                                                                                                                              [ 22%]
tests/test_pytest_plugin.py .                                                                                                                                                                                                         [ 22%]
tests/test_server.py ............                                                                                                                                                                                                     [ 29%]
tests/test_session.py .........................                                                                                                                                                                                       [ 43%]
tests/test_test.py .....                                                                                                                                                                                                              [ 46%]
tests/test_tmuxobject.py .....                                                                                                                                                                                                        [ 48%]
tests/test_window.py .......................                                                                                                                                                                                          [ 61%]
docs/index.md .............                                                                                                                                                                                                           [ 68%]
docs/quickstart.md .....................                                                                                                                                                                                              [ 80%]
docs/reference/properties.md .............                                                                                                                                                                                            [ 87%]
docs/topics/traversal.md .........                                                                                                                                                                                                    [ 92%]
README.md .............                                                                                                                                                                                                               [100%]

================================================================================================================= FAILURES ==================================================================================================================___________________________________________________________________________________________________ [doctest] libtmux.pane.Pane.send_keys ___________________________________________________________________________________________________150             .. versionchanged:: 0.14
151
152                Default changed from True to False.
153         literal : bool, optional
154             Send keys literally, default True.
155
156         Examples
157         --------
158         >>> pane = window.split_window(shell='sh')
159         >>> pane.capture_pane()
Expected:
    ['$']
Got:
    ['sh-5.1$']

/src/src/libtmux/pane.py:159: DocTestFailure
============================================================================================================= warnings summary ==============================================================================================================../home/arch/.cache/pypoetry/virtualenvs/libtmux-VsnhxLU2-py3.10/lib/python3.10/site-packages/_pytest/cacheprovider.py:433
  /home/arch/.cache/pypoetry/virtualenvs/libtmux-VsnhxLU2-py3.10/lib/python3.10/site-packages/_pytest/cacheprovider.py:433: PytestCacheWarning: cache could not write path /src/.pytest_cache/v/cache/nodeids
    config.cache.set("cache/nodeids", sorted(self.cached_nodeids))

../home/arch/.cache/pypoetry/virtualenvs/libtmux-VsnhxLU2-py3.10/lib/python3.10/site-packages/_pytest/cacheprovider.py:387
  /home/arch/.cache/pypoetry/virtualenvs/libtmux-VsnhxLU2-py3.10/lib/python3.10/site-packages/_pytest/cacheprovider.py:387: PytestCacheWarning: cache could not write path /src/.pytest_cache/v/cache/lastfailed
    config.cache.set("cache/lastfailed", self.lastfailed)

../home/arch/.cache/pypoetry/virtualenvs/libtmux-VsnhxLU2-py3.10/lib/python3.10/site-packages/_pytest/stepwise.py:52
  /home/arch/.cache/pypoetry/virtualenvs/libtmux-VsnhxLU2-py3.10/lib/python3.10/site-packages/_pytest/stepwise.py:52: PytestCacheWarning: cache could not write path /src/.pytest_cache/v/cache/stepwise
    session.config.cache.set(STEPWISE_CACHE_DIR, [])

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
========================================================================================================== short test summary info ==========================================================================================================FAILED src/libtmux/pane.py::libtmux.pane.Pane.send_keys
============================================================================================ 1 failed, 179 passed, 3 warnings, 2 rerun in 42.16s ============================================================================================

@zappolowski
Copy link
Contributor Author

You've run the tests on libtmux, but the issue is on tmuxp. 😃

But yeah, that error I also get on libtmux but I couldn't figure out how to make it work properly. As it's an easy to spot false-positive I'm fine with ignoring it (for me locally). I think it works on Ubuntu as they switched to using /bin/sh -> dash some years ago. On Arch /bin/sh -> bash which uses PS1=\s-\v$ .

@tony tony force-pushed the fix/do-not-assume-bash-is-default-shell branch from b1b3775 to e2b8600 Compare November 5, 2022 14:49
@tony
Copy link
Member

tony commented Nov 5, 2022

Edit2: A note on running the containers. As /src is mounted read-only you have to set in-project = false in poetry.toml.

I removed in-project = true from poetry.toml in libtmux and tmuxp and rebased the PRs

If you're in the PR's branches checked out:

git pull --rebase --autostash

@tony
Copy link
Member

tony commented Nov 5, 2022

Those podman and buildah commands were nifty.

@tony
Copy link
Member

tony commented Nov 5, 2022

Do you have a list of which tests you issues with when you use fish? At the moment, here's what I get:

FAILED tests/workspace/test_builder.py::test_load_workspace_enter[pane_enter_default_shortform] - libtmux.exc.WaitTimeout
FAILED tests/workspace/test_builder.py::test_load_workspace_enter[pane_enter_default_longform] - libtmux.exc.WaitTimeout
FAILED tests/workspace/test_builder.py::test_load_workspace_enter[pane_command_enter_default_shortform] - libtmux.exc.WaitTimeout
FAILED tests/workspace/test_builder.py::test_load_workspace_enter[pane_command_enter_default_longform] - libtmux.exc.WaitTimeout
FAILED tests/workspace/test_builder.py::test_load_workspace_sleep[command_level_sleep_shortform] - assert '___4___' in 'Welcome to fish, the friendly interactive shell\nType help for instructions on how to use fish\nt@t /h/t/w/p/tmuxp (m..._" echo "___$((1 + 3))___"\n          ^\nt@t /h/t/w/p/tmuxp (master)>  echo "___$((1 + 5))_...
FAILED tests/workspace/test_builder.py::test_load_workspace_sleep[command_level_pane_sleep_longform] - assert '___4___' in 'Welcome to fish, the friendly interactive shell\nType help for instructions on how to use fish\nt@t /h/t/w/p/tmuxp (m..._" echo "___$((1 + 3))___"\n          ^\nt@t /h/t/w/p/tmuxp (master)>  echo "___$((1 + 5))_...
FAILED tests/workspace/test_builder.py::test_load_workspace_sleep[pane_sleep_shortform] - assert '___4___' in 'Welcome to fish, the friendly interactive shell\nType help for instructions on how to use fish\nt@t /h/t/w/p/tmuxp (m...lease use \'((1 + 3))\'.\n echo "___$((1 + 3))___"\n          ^\nt@t /h/t/w/p/tmuxp (master...
FAILED tests/workspace/test_builder.py::test_load_workspace_sleep[pane_sleep_longform] - assert '___4___' in 'Welcome to fish, the friendly interactive shell\nType help for instructions on how to use fish\nt@t /h/t/w/p/tmuxp (m...lease use \'((1 + 3))\'.\n echo "___$((1 + 3))___"\n          ^\nt@t /h/t/w/p/tmuxp (master...
FAILED tests/workspace/test_builder.py::test_load_workspace_sleep[shell_before_before_command_level] - assert '___4___' in 'Welcome to fish, the friendly interactive shell\nType help for instructions on how to use fish\nt@t /h/t/w/p/tmuxp (m...lease use \'((1 + 3))\'.\n echo "___$((1 + 3))___"\n          ^\nt@t /h/t/w/p/tmuxp (master...

@zappolowski
Copy link
Contributor Author

zappolowski commented Nov 5, 2022

Yes, these are the same tests failing for me.

`tmux` uses the default login shell and thus tests might fail if this is
not bash (or something compatible). Explicitly stating which shell to
use circumvents issues arising from accidentally using another shell.
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

2 participants