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

.github/workflows/build.yaml

@@ -147,3 +147,56 @@ 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
+        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/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,22 @@
 messaging protocol correctly according to the specification.
 """
 
+import os
 import unittest
 import jupyter_kernel_test as jkt
 import subprocess
 from pathlib import Path
 
+# 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,47 +31,35 @@ 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"
-
     # Override test_execute_stdout - SQL kernels don't produce stdout
     def test_execute_stdout(self):
         """SQL kernels produce execute_result, not stdout streams."""
         # Skip this test for SQL kernels - they don't produce stdout
         # 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}")
-
-        super().setUp()
-
     # 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 +195,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,14 +243,29 @@ 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
+    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
+
+    # Build kernel (once for the whole module; individual tests no longer
+    # rebuild it in setUp).
     repo_root = Path(__file__).parent.parent.parent
     result = subprocess.run(
         ["cargo", "build", "--bin", "ggsql-jupyter"],
@@ -259,7 +284,7 @@ def setup_module():
     # Create kernel spec
     kernel_spec = {
         "argv": [str(binary_path), "-f", "{connection_file}"],
-        "display_name": "ggsql",
+        "display_name": KERNEL_NAME,
         "language": "ggsql",
     }
 
@@ -268,15 +293,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 +314,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

ggsql-jupyter/tests/test_integration.py

@@ -44,6 +44,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 +64,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