From 964e6b184110285d27617622dab698c0f8552954 Mon Sep 17 00:00:00 2001 From: Jongwook Choi Date: Sat, 14 Oct 2023 19:16:44 -0400 Subject: [PATCH 1/4] ci: run CI jobs on all combination of py37-py312 and three OS's --- .github/workflows/test.yml | 69 +++++++++++++++++++------------------- tox.ini | 4 ++- 2 files changed, 37 insertions(+), 36 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index f3b83a1e..bcd42875 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -27,37 +27,31 @@ jobs: strategy: fail-fast: false matrix: - python-version: ['3.10', '3.11', '3.12-dev'] + python-version: ['3.12', '3.11', '3.10', '3.9', '3.8', '3.7'] os: ['ubuntu-latest', 'macos-latest', 'windows-latest'] + exclude: + - os: 'ubuntu-latest' + python-version: '3.7' include: - - os: ubuntu-20.04 - python-version: '3.7' - NIGHTLY: nvim-linux64.tar.gz - NVIM_BIN_PATH: nvim-linux64/bin - EXTRACT: tar xzf - - os: ubuntu-latest - python-version: '3.8' - NIGHTLY: nvim-linux64.tar.gz - NVIM_BIN_PATH: nvim-linux64/bin - EXTRACT: tar xzf - - os: ubuntu-latest - python-version: '3.9' - NIGHTLY: nvim-linux64.tar.gz - NVIM_BIN_PATH: nvim-linux64/bin - EXTRACT: tar xzf - - os: ubuntu-latest - NIGHTLY: nvim-linux64.tar.gz - NVIM_BIN_PATH: nvim-linux64/bin - EXTRACT: tar xzf - - os: macos-latest - NIGHTLY: nvim-macos.tar.gz - NVIM_BIN_PATH: nvim-macos/bin - EXTRACT: tar xzf - - os: windows-latest - NIGHTLY: nvim-win64.zip - NVIM_BIN_PATH: nvim-win64/bin - EXTRACT: unzip + - os: 'ubuntu-20.04' + python-version: '3.7' + NIGHTLY: nvim-linux64.tar.gz + NVIM_BIN_PATH: nvim-linux64/bin + EXTRACT: tar xzf + - os: 'ubuntu-latest' + NIGHTLY: nvim-linux64.tar.gz + NVIM_BIN_PATH: nvim-linux64/bin + EXTRACT: tar xzf + - os: 'macos-latest' + NIGHTLY: nvim-macos.tar.gz + NVIM_BIN_PATH: nvim-macos/bin + EXTRACT: tar xzf + - os: 'windows-latest' + NIGHTLY: nvim-win64.zip + NVIM_BIN_PATH: nvim-win64/bin + EXTRACT: unzip + name: "test (python ${{ matrix.python-version }}, ${{ matrix.os }})" runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v3 @@ -66,12 +60,6 @@ jobs: cache: 'pip' python-version: ${{ matrix.python-version }} - - name: install neovim - run: | - curl -LO 'https://github.com/neovim/neovim/releases/download/nightly/${{ matrix.NIGHTLY }}' - ${{ matrix.EXTRACT }} ${{ matrix.NIGHTLY }} - echo '${{ runner.os }}' - - name: update path (bash) if: runner.os != 'Windows' run: echo "$(pwd)/${{ matrix.NVIM_BIN_PATH }}" >> $GITHUB_PATH @@ -80,16 +68,27 @@ jobs: if: runner.os == 'Windows' run: echo "$(pwd)/${{ matrix.NVIM_BIN_PATH }}" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append + - name: install neovim + run: | + curl -LO 'https://github.com/neovim/neovim/releases/download/nightly/${{ matrix.NIGHTLY }}' + ${{ matrix.EXTRACT }} ${{ matrix.NIGHTLY }} + echo '${{ runner.os }}' + nvim --version + - name: install dependencies run: | python3 -m pip install -U pip python3 -m pip install tox tox-gh-actions + - name: check neovim + run: | + python3 -m pip install -e . # install pynvim + nvim --headless --clean -c 'checkhealth | %+print | q' + - name: test with tox run: | echo $PATH which nvim - nvim --version which -a python3 python3 --version tox run diff --git a/tox.ini b/tox.ini index af37b91f..a57a7909 100644 --- a/tox.ini +++ b/tox.ini @@ -29,8 +29,10 @@ deps = # setenv = # cov: PYTEST_ADDOPTS=--cov=. {env:PYTEST_ADDOPTS:} # passenv = PYTEST_ADDOPTS + +# Note: Use python instead of python3 due to tox-dev/tox#2801 commands = - python3 -m pytest --color yes -s -vv {posargs} + python -m pytest --color yes -s --timeout 5 -vv {posargs} [testenv:checkqa] deps = From fd4247ced2b536c82fe76c52a6a6042eebb31ad4 Mon Sep 17 00:00:00 2001 From: Jongwook Choi Date: Sat, 14 Oct 2023 17:26:55 -0400 Subject: [PATCH 2/4] fix: do not leak resources across tests so as to prevent side effects Problem: Across unit tests, custom path_hook reigstered globally by ScriptHost globally was not being cleared up properly. This results in leakage of internal resources and dangling access to them (such as asyncio event loops that was already closed and no longer valid in other test case instances). More specifically, the asyncio EventLoop were never closed, which can result in "Event loop is closed" errors upon garbage collection of internal transport objects (during cleaning up pytest sessions). Solution: (1) Always call ScriptHost.teardown() when done using it. (2) Make sure all internal resources (event loops, transport channels, etc.) are closed by closing the embedded neovim instance. --- pynvim/msgpack_rpc/event_loop/asyncio.py | 8 +++++++- test/conftest.py | 20 ++++++++++++++++++-- test/test_host.py | 21 ++++++++++++++------- 3 files changed, 39 insertions(+), 10 deletions(-) diff --git a/pynvim/msgpack_rpc/event_loop/asyncio.py b/pynvim/msgpack_rpc/event_loop/asyncio.py index cea7298c..a506c927 100644 --- a/pynvim/msgpack_rpc/event_loop/asyncio.py +++ b/pynvim/msgpack_rpc/event_loop/asyncio.py @@ -14,7 +14,7 @@ import sys from collections import deque from signal import Signals -from typing import Any, Callable, Deque, List +from typing import Any, Callable, Deque, List, Optional from pynvim.msgpack_rpc.event_loop.base import BaseEventLoop @@ -37,6 +37,8 @@ class AsyncioEventLoop(BaseEventLoop, asyncio.Protocol, """`BaseEventLoop` subclass that uses `asyncio` as a backend.""" _queued_data: Deque[bytes] + if os.name != 'nt': + _child_watcher: Optional['asyncio.AbstractChildWatcher'] def connection_made(self, transport): """Used to signal `asyncio.Protocol` of a successful connection.""" @@ -78,6 +80,7 @@ def _init(self) -> None: self._queued_data = deque() self._fact = lambda: self self._raw_transport = None + self._child_watcher = None def _connect_tcp(self, address: str, port: int) -> None: coroutine = self._loop.create_connection(self._fact, address, port) @@ -145,6 +148,9 @@ def _close(self) -> None: if self._raw_transport is not None: self._raw_transport.close() self._loop.close() + if self._child_watcher is not None: + self._child_watcher.close() + self._child_watcher = None def _threadsafe_call(self, fn: Callable[[], Any]) -> None: self._loop.call_soon_threadsafe(fn) diff --git a/test/conftest.py b/test/conftest.py index 85e56ab6..9bbd5295 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -1,5 +1,9 @@ +"""Configs for pytest.""" + +import gc import json import os +from typing import Generator import pytest @@ -9,7 +13,10 @@ @pytest.fixture -def vim() -> pynvim.Nvim: +def vim() -> Generator[pynvim.Nvim, None, None]: + """Create an embedded, sub-process Nvim fixture instance.""" + editor: pynvim.Nvim + child_argv = os.environ.get('NVIM_CHILD_ARGV') listen_address = os.environ.get('NVIM_LISTEN_ADDRESS') if child_argv is None and listen_address is None: @@ -28,4 +35,13 @@ def vim() -> pynvim.Nvim: assert listen_address is not None and listen_address != '' editor = pynvim.attach('socket', path=listen_address) - return editor + try: + yield editor + + finally: + # Ensure all internal resources (pipes, transports, etc.) are always + # closed properly. Otherwise, during GC finalizers (__del__) will raise + # "Event loop is closed" error. + editor.close() + + gc.collect() # force-run GC, to early-detect potential leakages diff --git a/test/test_host.py b/test/test_host.py index 016d48b7..18cff327 100644 --- a/test/test_host.py +++ b/test/test_host.py @@ -11,15 +11,19 @@ def test_host_imports(vim): h = ScriptHost(vim) - assert h.module.__dict__['vim'] - assert h.module.__dict__['vim'] == h.legacy_vim - assert h.module.__dict__['sys'] + try: + assert h.module.__dict__['vim'] + assert h.module.__dict__['vim'] == h.legacy_vim + assert h.module.__dict__['sys'] + finally: + h.teardown() def test_host_import_rplugin_modules(vim): # Test whether a Host can load and import rplugins (#461). # See also $VIMRUNTIME/autoload/provider/pythonx.vim. h = Host(vim) + plugins: Sequence[str] = [ # plugin paths like real rplugins os.path.join(__PATH__, "./fixtures/simple_plugin/rplugin/python3/simple_nvim.py"), os.path.join(__PATH__, "./fixtures/module_plugin/rplugin/python3/mymodule/"), @@ -56,7 +60,10 @@ def test_host_async_error(vim): def test_legacy_vim_eval(vim): h = ScriptHost(vim) - assert h.legacy_vim.eval('1') == '1' - assert h.legacy_vim.eval('v:null') is None - assert h.legacy_vim.eval('v:true') is True - assert h.legacy_vim.eval('v:false') is False + try: + assert h.legacy_vim.eval('1') == '1' + assert h.legacy_vim.eval('v:null') is None + assert h.legacy_vim.eval('v:true') is True + assert h.legacy_vim.eval('v:false') is False + finally: + h.teardown() From 6ab90aad933ddbd97e0f65945f994acff4b6e1c9 Mon Sep 17 00:00:00 2001 From: Jongwook Choi Date: Sat, 14 Oct 2023 20:25:54 -0400 Subject: [PATCH 3/4] fix: EOF error on piped stderr being closed on Windows Problem: An EOF error happens when creating a subprocess Nvim instance on Windows. All CI tests are failing, and `attach('child', ...)` cannot be run. Solution: Ignore pipe_connection_lost error, and do not close the asyncio event loop. Since embedded nvim only expects to use stdin and stdout only as a msgpack-RPC channel, it's fine to ignore broken pipes on the stderr. Fixes #505 --- pynvim/msgpack_rpc/event_loop/asyncio.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pynvim/msgpack_rpc/event_loop/asyncio.py b/pynvim/msgpack_rpc/event_loop/asyncio.py index a506c927..164173b8 100644 --- a/pynvim/msgpack_rpc/event_loop/asyncio.py +++ b/pynvim/msgpack_rpc/event_loop/asyncio.py @@ -60,12 +60,17 @@ def data_received(self, data: bytes) -> None: def pipe_connection_lost(self, fd, exc): """Used to signal `asyncio.SubprocessProtocol` of a lost connection.""" + debug("pipe_connection_lost: fd = %s, exc = %s", fd, exc) + if os.name == 'nt' and fd == 2: # stderr + # On windows, ignore piped stderr being closed immediately (#505) + return self._on_error(exc.args[0] if exc else 'EOF') def pipe_data_received(self, fd, data): """Used to signal `asyncio.SubprocessProtocol` of incoming data.""" if fd == 2: # stderr fd number - self._on_stderr(data) + # Ignore stderr message, log only for debugging + debug("stderr: %s", str(data)) elif self._on_data: self._on_data(data) else: From 056f6f95a3214937ea878bc21ff896d0807deda0 Mon Sep 17 00:00:00 2001 From: Jongwook Choi Date: Sat, 14 Oct 2023 02:50:47 -0400 Subject: [PATCH 4/4] fix: ignore flaky OSError on windows Problem: ``` > os.unlink(fname) E PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\RUNNER~1\\AppData\\Local\\Temp\\tmpugb75_rw' ``` --- test/test_vim.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/test_vim.py b/test/test_vim.py index 2ea5a103..dc280fa1 100644 --- a/test/test_vim.py +++ b/test/test_vim.py @@ -1,4 +1,3 @@ -# -*- coding: utf-8 -*- import os import tempfile from typing import Any @@ -31,7 +30,10 @@ def test_command(vim: Nvim) -> None: assert os.path.isfile(fname) with open(fname) as f: assert f.read() == 'testing\npython\napi\n' - os.unlink(fname) + try: + os.unlink(fname) + except OSError: + pass # on windows, this can be flaky; ignore it def test_command_output(vim: Nvim) -> None: