Fix stale, hanging, and unrun ggsql-jupyter protocol tests

.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

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
 

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.

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()

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

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,15 +322,15 @@ 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",
             "kernelspec",
             "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