diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 4b26b1c3..0e3b2ba1 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -147,3 +147,61 @@ jobs: - name: Build WASM package run: ./ggsql-wasm/build-wasm.sh --skip-opt + + jupyter-protocol-tests: + # The Python suite under ggsql-jupyter/tests/ that drives the kernel over + # a real ZMQ connection — the one thing the Rust unit tests above cannot + # reach (wire framing, HMAC signing, busy/idle ordering). See + # ggsql-jupyter/CLAUDE.md, "Testing". + runs-on: ubuntu-latest + # continue-on-error until posit-dev/ggsql#556 (a heartbeat-socket panic + # in the vendored zeromq crate, unrelated to this suite) is fixed: that + # bug crashes the kernel process in a large fraction of runs, which + # would otherwise make this job flaky-red rather than a real signal. + # Remove this once #556 lands. + continue-on-error: true + + steps: + - name: Check out repository + uses: actions/checkout@v4 + + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: "22" + + - name: Install tree-sitter-cli + # parser.c is generated from grammar.js by the tree-sitter-ggsql + # build script and is not committed, so this is needed on every + # fresh checkout — see /CLAUDE.md, "Building". + run: npm install -g tree-sitter-cli + + - name: Install Rust + uses: dtolnay/rust-toolchain@stable + + - name: Caching + uses: Swatinem/rust-cache@v2 + with: + shared-key: ${{ runner.os }}-build + cache-on-failure: true + save-if: ${{ github.ref == 'refs/heads/main' }} + + - name: Build ggsql-jupyter + run: cargo build --bin ggsql-jupyter + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" + + - name: Install Python test dependencies + run: pip install -r ggsql-jupyter/tests/requirements.txt + + - name: Run ggsql-jupyter protocol tests + working-directory: ggsql-jupyter/tests + # The grammar was already generated by the "Build ggsql-jupyter" step + # above; the fixtures' own `cargo build` calls (see conftest.py) are + # just up-to-date checks and don't need tree-sitter-cli again. + env: + GGSQL_SKIP_GENERATE: "1" + run: pytest test_integration.py test_compliance.py -v diff --git a/ggsql-jupyter/CLAUDE.md b/ggsql-jupyter/CLAUDE.md index 7a099b22..c770aef6 100644 --- a/ggsql-jupyter/CLAUDE.md +++ b/ggsql-jupyter/CLAUDE.md @@ -215,7 +215,7 @@ pip install -r requirements.txt pytest ``` -`test_compliance.py` verifies handler coverage (`execute_request`, `kernel_info_request`, `is_complete_request`, `shutdown_request`); `test_integration.py` drives a real kernel via `jupyter_client`. +`test_compliance.py` verifies handler coverage (`execute_request`, `kernel_info_request`, `is_complete_request`, `shutdown_request`); `test_integration.py` drives a real kernel via `jupyter_client`. Both run in CI (`jupyter-protocol-tests` job in `/.github/workflows/build.yaml`). ## See also diff --git a/ggsql-jupyter/tests/README.md b/ggsql-jupyter/tests/README.md index 7729a696..6990a68f 100644 --- a/ggsql-jupyter/tests/README.md +++ b/ggsql-jupyter/tests/README.md @@ -70,6 +70,8 @@ Run official Jupyter kernel compliance tests: ```bash # From ggsql-jupyter/tests/ directory pytest test_compliance.py -v - -# Note: This will install the kernel spec temporarily ``` + +`test_compliance.py` installs its kernelspec under a scratch `JUPYTER_DATA_DIR` +it creates and tears down itself, using the name `ggsql-test` — running it +never touches a real `ggsql` kernelspec you may have installed. diff --git a/ggsql-jupyter/tests/conftest.py b/ggsql-jupyter/tests/conftest.py new file mode 100644 index 00000000..b1d4865f --- /dev/null +++ b/ggsql-jupyter/tests/conftest.py @@ -0,0 +1,38 @@ +"""Shared fixtures for the ggsql-jupyter protocol test suite. + +`test_integration.py` (pytest) and `test_compliance.py` (unittest-style +setup_module) both need the kernel binary, so the build lives here once +rather than duplicated in each file. CI's own build step already generates +the grammar and compiles the binary before pytest runs; calling this again +from a fixture is a cheap up-to-date check, not a rebuild. +""" + +import subprocess +from pathlib import Path + +import pytest + + +def build_kernel_binary() -> str: + """Build ggsql-jupyter and return the path to its binary.""" + repo_root = Path(__file__).parent.parent.parent + result = subprocess.run( + ["cargo", "build", "--bin", "ggsql-jupyter"], + cwd=repo_root / "ggsql-jupyter", + capture_output=True, + text=True, + ) + if result.returncode != 0: + raise RuntimeError(f"Failed to build kernel: {result.stderr}") + + binary_path = repo_root / "target" / "debug" / "ggsql-jupyter" + if not binary_path.exists(): + raise RuntimeError(f"Kernel binary not found at {binary_path}") + + return str(binary_path) + + +@pytest.fixture(scope="session") +def kernel_binary() -> str: + """Build and return path to ggsql-jupyter binary, once per session.""" + return build_kernel_binary() diff --git a/ggsql-jupyter/tests/requirements.txt b/ggsql-jupyter/tests/requirements.txt index b12d1037..a1eb560d 100644 --- a/ggsql-jupyter/tests/requirements.txt +++ b/ggsql-jupyter/tests/requirements.txt @@ -3,7 +3,3 @@ jupyter-client>=8.0.0 jupyter-kernel-test>=0.7.0 pytest>=7.0.0 pytest-asyncio>=0.21.0 - -# For manual testing and debugging -jupyterlab>=4.0.0 -ipykernel>=6.0.0 diff --git a/ggsql-jupyter/tests/test_compliance.py b/ggsql-jupyter/tests/test_compliance.py index e5e48b1b..982cb840 100644 --- a/ggsql-jupyter/tests/test_compliance.py +++ b/ggsql-jupyter/tests/test_compliance.py @@ -5,17 +5,24 @@ messaging protocol correctly according to the specification. """ +import os import unittest import jupyter_kernel_test as jkt import subprocess from pathlib import Path +from conftest import build_kernel_binary + +# Isolated from any real Jupyter install: setup_module points JUPYTER_DATA_DIR +# at a scratch directory before this name is ever installed or removed. +KERNEL_NAME = "ggsql-test" + class ggsqlKernelTests(jkt.KernelTests): """Compliance tests for ggsql-jupyter kernel.""" # Kernel name (will be overridden to use custom command) - kernel_name = "ggsql" + kernel_name = KERNEL_NAME # Language name language_name = "ggsql" @@ -26,10 +33,13 @@ class ggsqlKernelTests(jkt.KernelTests): # Code samples for testing code_hello_world = "SELECT 'Hello, World!' as greeting" - # Expected output pattern (for simple SELECT) - # Note: jupyter_kernel_test looks for this in text/plain output - # We may need to adjust based on actual output format - code_page_something = "SELECT 'something' as result" + # These have a real implementation behind them, so defining the sample + # lets jupyter_kernel_test's own inherited test exercise it directly + # rather than duplicating the assertions in a test we wrote ourselves. + code_generate_error = "SELECT * FROM nonexistent_table" + code_execute_result = [{"code": "SELECT 123 as num", "mime": "text/plain"}] + complete_code_samples = ["SELECT 1"] + incomplete_code_samples = ["SELECT (1"] # Override test_execute_stdout - SQL kernels don't produce stdout def test_execute_stdout(self): @@ -38,35 +48,60 @@ def test_execute_stdout(self): # They produce execute_result messages instead pass - def setUp(self): - """Build kernel before tests.""" - # Build the kernel - repo_root = Path(__file__).parent.parent.parent - result = subprocess.run( - ["cargo", "build", "--bin", "ggsql-jupyter"], - cwd=repo_root / "ggsql-jupyter", - capture_output=True, - text=True, - ) - if result.returncode != 0: - self.fail(f"Failed to build kernel: {result.stderr}") + # Everything below has no backing implementation in the kernel: there is + # no complete_request, inspect_request or history_request handler (see + # kernel.rs's message dispatch), `payload` is hardcoded to `[]` so there + # is no pager support, and nothing is ever emitted as `display_data` — + # results always go out as `execute_result`. Defining the sample + # attributes that would make these inherited tests run would exercise + # protocol features this kernel doesn't have, so they're overridden here + # to record that as a deliberate choice rather than a silent SkipTest. + def test_execute_stderr(self): + """No stream messages of any kind are ever emitted.""" + pass + + def test_completion(self): + """No complete_request handler exists.""" + pass + + def test_pager(self): + """`payload` is hardcoded to `[]`; there is no pager support.""" + pass + + def test_display_data(self): + """Results always go out as execute_result, never display_data.""" + pass - super().setUp() + def test_history(self): + """No history_request handler exists.""" + pass + + def test_inspect(self): + """No inspect_request handler exists.""" + pass # Test that kernel_info_request works def test_kernel_info(self): - """Test kernel_info_request returns correct information.""" + """Test kernel_info_request returns correct information. + + `get_non_kernel_info_reply` (jkt's own `execute_helper` uses it to + skip past an unsolicited reply and get to the one actually being + waited on) would hang here forever: it explicitly discards + `kernel_info_reply` messages, but that is the only reply this + request ever produces. `get_shell_msg` with a bounded timeout is + what jkt's own base `test_kernel_info` uses for the same request. + """ self.flush_channels() msg_id = self.kc.kernel_info() - reply = self.get_non_kernel_info_reply() + reply = self.kc.get_shell_msg(timeout=jkt.TIMEOUT) self.assertEqual(reply["msg_type"], "kernel_info_reply") content = reply["content"] self.assertEqual(content["status"], "ok") self.assertEqual(content["protocol_version"], "5.3") - self.assertEqual(content["implementation"], "ggsql") + self.assertEqual(content["implementation"], "ggsql-jupyter") # Language info lang_info = content["language_info"] @@ -202,15 +237,32 @@ def test_execute_input(self): # Test shutdown def test_shutdown(self): - """Test that shutdown works.""" - self.flush_channels() + """Test that shutdown works. + + `setUpClass`/`tearDownClass` own one kernel shared by every test + method in this class, so shutting *that* one down here would leave + nothing for whatever test runs next (unittest orders methods + alphabetically, so this would otherwise run before + `test_status_messages`). Start a throwaway kernel instead. + + `kc.shutdown()` sends `shutdown_request` on the *control* channel + (see `KernelClient.shutdown`'s docstring), and the reply comes back + on the same channel — not shell, despite the original version of + this test waiting on `get_shell_msg` and timing out here every time. + """ + from jupyter_client.manager import start_new_kernel - msg_id = self.kc.shutdown() - reply = self.kc.get_shell_msg(timeout=5) + km, kc = start_new_kernel(kernel_name=self.kernel_name) + try: + msg_id = kc.shutdown() + reply = kc.get_control_msg(timeout=5) - self.assertEqual(reply["msg_type"], "shutdown_reply") - self.assertEqual(reply["content"]["status"], "ok") - self.assertIn("restart", reply["content"]) + self.assertEqual(reply["msg_type"], "shutdown_reply") + self.assertEqual(reply["content"]["status"], "ok") + self.assertIn("restart", reply["content"]) + finally: + kc.stop_channels() + km.shutdown_kernel() # Test persistent state def test_persistent_state(self): @@ -233,33 +285,35 @@ def test_persistent_state(self): self.assertEqual(reply3["content"]["status"], "ok") +# Restored in teardown_module. Set before install so the kernelspec below +# never touches a developer's real Jupyter data directory. +_original_jupyter_data_dir = None +_scratch_data_dir = None + + # Configure kernel for testing def setup_module(): - """Setup module by installing kernel spec.""" + """Build the kernel once and install it into an isolated kernelspec.""" import tempfile import json - import os - # Build kernel - repo_root = Path(__file__).parent.parent.parent - result = subprocess.run( - ["cargo", "build", "--bin", "ggsql-jupyter"], - cwd=repo_root / "ggsql-jupyter", - capture_output=True, - text=True, - ) - if result.returncode != 0: - raise RuntimeError(f"Failed to build kernel: {result.stderr}") + global _original_jupyter_data_dir, _scratch_data_dir + + # Isolate JUPYTER_DATA_DIR before anything below can install or remove a + # kernelspec, so this suite can never clobber a developer's real "ggsql" + # kernel — nothing in the environment has to know to set this itself. + _original_jupyter_data_dir = os.environ.get("JUPYTER_DATA_DIR") + _scratch_data_dir = tempfile.mkdtemp(prefix="ggsql-jupyter-data-") + os.environ["JUPYTER_DATA_DIR"] = _scratch_data_dir - # Find binary - binary_path = repo_root / "target" / "debug" / "ggsql-jupyter" - if not binary_path.exists(): - raise RuntimeError(f"Kernel binary not found at {binary_path}") + # Build kernel (once for the whole module; individual tests no longer + # rebuild it in setUp). Shared with test_integration.py via conftest.py. + binary_path = build_kernel_binary() # Create kernel spec kernel_spec = { - "argv": [str(binary_path), "-f", "{connection_file}"], - "display_name": "ggsql", + "argv": [binary_path, "-f", "{connection_file}"], + "display_name": KERNEL_NAME, "language": "ggsql", } @@ -268,7 +322,7 @@ def setup_module(): with open(spec_dir / "kernel.json", "w") as f: json.dump(kernel_spec, f) - # Install kernel spec + # Install kernel spec (into the scratch JUPYTER_DATA_DIR set above) result = subprocess.run( [ "jupyter", @@ -276,7 +330,7 @@ def setup_module(): "install", "--user", "--name", - "ggsql", + KERNEL_NAME, str(spec_dir), ], capture_output=True, @@ -289,12 +343,22 @@ def setup_module(): def teardown_module(): - """Cleanup kernel spec after tests.""" + """Cleanup kernel spec after tests and restore JUPYTER_DATA_DIR.""" subprocess.run( - ["jupyter", "kernelspec", "remove", "-f", "ggsql"], + ["jupyter", "kernelspec", "remove", "-f", KERNEL_NAME], capture_output=True, ) + if _original_jupyter_data_dir is None: + os.environ.pop("JUPYTER_DATA_DIR", None) + else: + os.environ["JUPYTER_DATA_DIR"] = _original_jupyter_data_dir + + if _scratch_data_dir is not None: + import shutil + + shutil.rmtree(_scratch_data_dir, ignore_errors=True) + if __name__ == "__main__": # Run setup diff --git a/ggsql-jupyter/tests/test_integration.py b/ggsql-jupyter/tests/test_integration.py index 4bee0506..a409ec15 100644 --- a/ggsql-jupyter/tests/test_integration.py +++ b/ggsql-jupyter/tests/test_integration.py @@ -5,36 +5,16 @@ to verify correct behavior. """ +import contextlib import json import time import subprocess -import tempfile import os -from pathlib import Path import pytest from jupyter_client import KernelManager - -@pytest.fixture(scope="session") -def kernel_binary(): - """Build and return path to ggsql-jupyter binary.""" - # Build the kernel - repo_root = Path(__file__).parent.parent.parent - result = subprocess.run( - ["cargo", "build", "--bin", "ggsql-jupyter"], - cwd=repo_root / "ggsql-jupyter", - capture_output=True, - text=True, - ) - if result.returncode != 0: - pytest.fail(f"Failed to build kernel: {result.stderr}") - - # Find binary - binary_path = repo_root / "target" / "debug" / "ggsql-jupyter" - if not binary_path.exists(): - pytest.fail(f"Kernel binary not found at {binary_path}") - - return str(binary_path) +# kernel_binary is defined in conftest.py, shared with test_compliance.py so +# the kernel is built once rather than once per test file. def _launch(kernel_binary, extra_args=()): @@ -44,6 +24,13 @@ def _launch(kernel_binary, extra_args=()): try: # Use KernelManager to write connection file with proper ports km = KernelManager() + # We launch and kill the process ourselves below (km.start_kernel() + # is never called), so km's own liveness bookkeeping never gets set + # up: km.is_alive() would otherwise unconditionally report the + # kernel dead, since has_kernel is False. Telling it we don't own + # the kernel process makes wait_for_ready() rely purely on getting a + # real kernel_info_reply within its timeout, which is what we want. + km._owns_kernel = False km.write_connection_file() connection_file = km.connection_file @@ -57,15 +44,24 @@ def _launch(kernel_binary, extra_args=()): # Store the process in km for cleanup km._kernel_process = kernel_process - # Wait for kernel to be ready - time.sleep(3) - - # Check if process started successfully - if kernel_process.poll() is not None: - stdout, stderr = kernel_process.communicate() + # Wait for the kernel to actually be ready (heartbeat up) rather than + # guessing with a fixed sleep. Opened and closed here rather than + # left open, since callers that want a client build their own via + # km.client() (see the `client`/`console_client` fixtures below). + probe = km.client() + probe.start_channels() + try: + probe.wait_for_ready(timeout=10) + except RuntimeError: + probe.stop_channels() + stdout, stderr = (b"", b"") + if kernel_process.poll() is not None: + stdout, stderr = kernel_process.communicate() pytest.fail( f"Kernel failed to start:\nstdout: {stdout.decode()}\nstderr: {stderr.decode()}" ) + else: + probe.stop_channels() yield km @@ -87,15 +83,26 @@ def _launch(kernel_binary, extra_args=()): pass -@pytest.fixture +# Also usable as `with _launch_kernel(binary) as km:` outside of pytest's +# fixture machinery — see TestShutdown.test_shutdown_request below, which +# needs a kernel of its own rather than the session-scoped one. +_launch_kernel = contextlib.contextmanager(_launch) + + +@pytest.fixture(scope="session") def kernel_manager(kernel_binary): - """Create and start a kernel manager.""" + """Start one kernel manager, shared by every test in this session. + + Session-scoped so the suite doesn't pay kernel startup cost per test. + Any test that needs to shut down or otherwise damage its kernel — see + TestShutdown — must launch a private one instead of using this fixture. + """ yield from _launch(kernel_binary) -@pytest.fixture +@pytest.fixture(scope="session") def console_kernel_manager(kernel_binary): - """A kernel that believes it is a Positron console session. + """A session-scoped kernel that believes it is a Positron console session. `--session-mode` is what the extension's `createKernelSpec` appends, and it is authoritative over the session-id heuristic — so this is the only way to @@ -529,37 +536,32 @@ def test_status_busy_idle(self, client): class TestShutdown: """Test shutdown_request/reply messages.""" - def test_shutdown_request(self, kernel_manager): - """Test that kernel responds to shutdown_request.""" - kc = kernel_manager.client() - kc.start_channels() + def test_shutdown_request(self, kernel_binary): + """Test that kernel responds to shutdown_request. - try: - kc.wait_for_ready(timeout=10) + Launches its own kernel rather than taking the session-scoped + `kernel_manager` fixture: this test's whole point is to shut its + kernel down, which would take every other test in this module down + with it if it were the shared one. - # Send shutdown request on control channel - msg_id = kc.shutdown() - - # Try to get reply with a longer timeout + The reply comes back on the *control* channel — `kc.shutdown()` + sends `shutdown_request` there, per `KernelClient.shutdown`'s + docstring — not shell. + """ + with _launch_kernel(kernel_binary) as km: + kc = km.client() + kc.start_channels() try: - reply = kc.get_shell_msg(timeout=10) + kc.wait_for_ready(timeout=10) + + msg_id = kc.shutdown() + reply = kc.get_control_msg(timeout=10) assert reply["msg_type"] == "shutdown_reply" assert reply["content"]["status"] == "ok" assert "restart" in reply["content"] - except: - # If we can't get the reply, at least verify the kernel process is terminating - # Wait a bit for shutdown to process - time.sleep(2) - kernel_process = kernel_manager._kernel_process - # Process should either be terminated or terminating - if kernel_process.poll() is None: - # Still running, send explicit shutdown - kernel_process.terminate() - kernel_process.wait(timeout=5) - - finally: - kc.stop_channels() + finally: + kc.stop_channels() class TestExecuteInput: