test: cover the client content panes, mark the suite, and report coverage in CI (PyTest - 10)

.agents/context/testing.md

@@ -8,11 +8,18 @@ Two layers: **pytest** for Python unit tests (pure functions, server utils, wind
 pip install -r test-requirements.txt   # includes pytest, pytest-cov
 pytest                                 # runs the tracked suite under py/tests/
 pytest -m "not server"                 # skip tests that need a live server (CI default)
+pytest -m unit                         # fast loop while writing, ~2s
+pytest -m "not server" --cov=visdom --cov-report=term-missing   # what CI gates on
 ```
 
+`-q` is already in `addopts`, so passing another `-q` makes it `-qq` and hides the pass/fail
+summary. Add `-o addopts=""` when you want a usable `--collect-only` count.
+
 Config lives in `pyproject.toml` (`[tool.pytest.ini_options]`): discovery is scoped to
-`py/tests/` and `pythonpath = ["py"]` makes `import visdom` work without an editable install.
-Experimental `test_*.py` scripts in the repo root (and `test/`) are intentionally out of scope.
+`py/tests/`, and `pythonpath = ["py", "py/tests"]` makes both `import visdom` and
+`import testutils` work without an editable install. Because discovery is scoped by `testpaths`,
+experimental `test_*.py` scripts in the repo root (and `test/`) stay out of scope; `testutils/` is
+excluded via `norecursedirs` so helpers are importable but never collected.
 
 ## Run E2E / Visual Tests (Cypress)
 
@@ -38,17 +45,138 @@ Always use port `8098` and `-env_path /tmp` for isolation.
 
 ## Writing Python Tests
 
-- Place in `py/tests/`, name files `test_*.py`, classes `Test*` (unittest `TestCase` or plain
-  pytest functions both work — pytest auto-discovers unittest).
-- Keep them hermetic (no running server). A test that genuinely needs a live server must be
-  marked `@pytest.mark.server` so CI can deselect it.
-- Start with simple hermetic tests (e.g., `test_smoke.py`) and add focused unit tests for
-  server/window/env lifecycle code as coverage grows.
+### Where a test goes
+
+```
+py/tests/
+  conftest.py        shared fixtures, auto-loaded by pytest
+  testutils/         importable helpers (fakes, payload builders, HTTP base class)
+  unit/              pure logic: no Application, no I/O beyond tmp_path
+  integration/       in-process Application, real HTTP, or handler dispatch
+```
+
+`py/tests/` has **no** `__init__.py` on purpose — `setup.py` runs `find_packages(where="py")`,
+so a package there would ship a top-level `tests` distribution to users. `testutils/` is a
+package and is reachable because `py/tests` is on `pythonpath`.
+
+Everything under `py/tests/` must be **hermetic and collectable**: no externally launched
+server, no browser, no assertion a human has to make. A script that needs a live server and is
+judged by looking at the UI goes in `example/manual/` instead — see
+`example/manual/visual_check.py`. Pixel correctness is Playwright's and Cypress's job, not
+pytest's.
+
+- Name a file after what it covers — `integration/window_types.py`, not
+  `integration/test_window_types.py`. The `unit/` and `integration/` directories already say these
+  are tests, so the filename does not repeat it; `python_files = ["*.py"]` in `pyproject.toml`
+  collects them, and `norecursedirs` keeps `testutils/` importable but uncollected. Test
+  *functions* and `Test*` classes still need their usual prefixes.
+- **Which style you use depends on whether the test needs HTTP.**
+
+  | Test needs | Write | Why |
+  |---|---|---|
+  | no `Application`, or a handler object | plain `def test_*()` functions | fixtures and `parametrize` both work |
+  | a real HTTP round trip | a `VisdomHTTPTestCase` subclass | `tornado.testing.AsyncHTTPTestCase` is a `unittest.TestCase`, and that is what starts the app |
+
+  **pytest cannot inject fixtures into `TestCase` methods** — `def test_x(self, app)` fails, and
+  only autouse fixtures reach them. `@pytest.mark.parametrize` does not work on them either; use
+  a small `_assert_*` helper called from several one-line test methods instead. A module-level
+  `pytestmark = pytest.mark.integration` **does** apply to `TestCase` classes, so always set one.
+- Keep them hermetic. A test that needs an **externally launched** server must be marked
+  `@pytest.mark.server` so CI can deselect it; nothing in the tracked suite needs one today.
+
+### Shared fixtures (`py/tests/conftest.py`)
+
+| Fixture | Gives you |
+|---|---|
+| `env_path` | disposable environment directory |
+| `store` / `spy_store` | `JSONStore` / one that records backend calls |
+| `app` / `app_factory` | `Application` on a temp `env_path`; factory for reload assertions |
+| `handler` / `app_handler` | duck-typed handler, standalone or sharing an `Application`'s state |
+| `fake_socket` | records `write_message`; `.commands()` and `.last(cmd)` for assertions |
+| `offline_client` | `Visdom(send=False)` — never opens a connection |
+| `capture_send` | runs a client call and returns the payload it would have sent |
+
+`reset_warn_once` is autouse: `shared_utils.warn_once` dedupes against a module-level set, so
+without it a warning raised by one test silently suppresses the same warning in another.
+
+### HTTP tests
+
+Subclass `testutils.VisdomHTTPTestCase`. It starts the app in-process on an ephemeral port, gives
+every test a fresh `env_path` that is cleaned up in `tearDown`, and provides `post_json`,
+`create_window`, `create_text_window`, `update`, `close_window`, `win_exists`, `get_win_data`,
+`get_envs`, `save` and `panes`, on top of `AsyncHTTPTestCase`'s own `fetch`. Override the
+`app_kwargs` class attribute to vary server configuration:
+
+```python
+class TestReadonlyRoutes(VisdomHTTPTestCase):
+    app_kwargs = {"readonly": True}
+```
+
+`AsyncHTTPTestCase` already runs the `Application` in-process on its own `IOLoop`, driving each
+request through `io_loop.run_sync`. **Do not replace it with a background thread, a hand-rolled
+`asyncio` loop, or an out-of-process server** — none of that buys anything, and it was tried and
+reverted. The `TestCase` style is the accepted cost of using it.
+
+Because fixtures cannot reach these tests, anything shared goes on the class: `self.env_path` for
+the temp directory, and a small base class between `VisdomHTTPTestCase` and your test classes for
+helpers several of them need (see `WindowTypeTestCase` in `integration/window_types.py`).
+Need a second `Application` over the same directory, for a reload assertion? Construct it directly
+with `Application(port=8097, env_path=self.env_path)` — `app_factory` is not available here.
+
+### Markers
+
+Registered in `pyproject.toml`:
+
+| Marker | Means | In the CI default run? |
+|---|---|---|
+| `unit` | no `Application`, no I/O beyond `tmp_path` | yes, and in the fast gate job |
+| `integration` | in-process `Application`, real HTTP, or handler dispatch | yes |
+| `slow` | takes more than a couple of seconds | yes, deselectable locally |
+| `server` | needs an **externally launched** visdom on a real port | **no** |
+
+Set `pytestmark = pytest.mark.unit` or `pytest.mark.integration` at the top of **every** file; it
+works for plain functions and `TestCase` classes alike. This is not decoration — CI runs `-m unit`
+as a gate, so an unmarked file is in neither job and is effectively only covered by the slower run.
+
+The invariant to preserve: **`-m unit` and `-m integration` must sum to the whole suite.**
+
+```bash
+pytest py/tests --collect-only -q -o addopts="" | tail -1   # 1502
+pytest -m unit --collect-only -q -o addopts="" | tail -1    # 1072
+pytest -m integration --collect-only -q -o addopts="" | tail -1  # 430
+```
+
+If those stop adding up, a file lost its marker.
+
+Nothing in the tracked suite is marked `server`; everything under `py/tests/` is hermetic by
+design. A script that needs a live server belongs in `example/manual/`.
+
+### Keeping the suite fast
+
+The whole suite runs in **under ten seconds**. It is worth keeping it there, and the way it stops
+being there is usually a client built without `use_incoming_socket=False`: that client waits out a
+socket connect timeout, costing 6.3 seconds per test. `integration/experiment_log_handler.py` once
+spent 38 of the suite's 43 seconds that way.
+
+Use the `offline_client` fixture rather than constructing a `Visdom` by hand. If you must build
+one, pass `send=False, use_incoming_socket=False`. `pytest --durations=10` shows the outliers.
 
 ## CI
 
-- `python-tests.yml` runs `pytest -m "not server"` on a Python version matrix for every pull
-  request (and on pushes to master/dev)
+- `python-tests.yml` runs two jobs on every pull request (and on pushes to master/dev):
+  - **`unit`** — `pytest -m unit` on 3.12 only. Finishes in a couple of seconds and gates the
+    matrix job, so an obvious break fails before three torch installs happen.
+  - **`pytest`** — `pytest -m "not server"` on a 3.12/3.13 matrix, with
+    `--cov=visdom --cov-report=term-missing`.
+- Coverage is **reported, not enforced**. There is no `--cov-fail-under` yet: it needs a number the
+  whole codebase can hold, and picking one is its own decision. Current total is **84%**, and the
+  report exists so that number can be chosen from real data rather than guessed.
+- What a floor would have to account for, whenever it is set: `loggers/sklearn.py` (108 statements)
+  and `pytorch.py` (39) sit at 0% by design — `autolog()` monkey-patches every `sklearn` estimator
+  with no un-patch API, so testing it would corrupt the session. Either omit them in
+  `[tool.coverage.run]` or pick a floor that expects them to be missing.
+- The `--cov` flags live in the workflow, **not** in `pyproject.toml`'s `addopts`, which would make
+  every local `pytest` hard-require pytest-cov and slow down single-file runs.
 - Visual regression compares PR screenshots against base branch
 - `update-js-build-files.yml` auto-compiles JS on master
 - `pypi.yml` publishes to PyPI when VERSION changes

.github/workflows/python-tests.yml

@@ -8,8 +8,30 @@ on:
       - dev
 
 jobs:
+  # Fast gate: the unit tests need no Application and no HTTP, so they finish in
+  # about a second. Running them first means an obvious break fails the workflow
+  # before the matrix below spends time installing torch three times over.
+  unit:
+    name: "unit"
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout Repository
+        uses: actions/checkout@v7.0.1
+      - name: Set up Python
+        uses: actions/setup-python@v7
+        with:
+          python-version: "3.12"
+      - name: Install package and test dependencies
+        run: |
+          python -m pip install --upgrade pip
+          python -m pip install -e .
+          python -m pip install -r test-requirements.txt
+      - name: Run unit tests
+        run: pytest -m unit
+
   pytest:
     name: "pytest (Python ${{ matrix.python-version }})"
+    needs: unit
     runs-on: ubuntu-latest
     strategy:
       fail-fast: false
@@ -27,5 +49,15 @@ jobs:
           python -m pip install --upgrade pip
           python -m pip install -e .
           python -m pip install -r test-requirements.txt
-      - name: Run pytest
-        run: pytest -m "not server"
+      # Coverage is reported, not enforced. A --cov-fail-under threshold is a
+      # separate decision -- it needs a number the whole codebase can actually
+      # hold, and picking one belongs in its own PR rather than riding along
+      # with this suite work. The report is here so that number can be chosen
+      # from real data.
+      # The flags live here rather than in pyproject's addopts, which would make
+      # every local pytest hard-require pytest-cov.
+      - name: Run pytest with coverage
+        run: >-
+          pytest -m "not server"
+          --cov=visdom
+          --cov-report=term-missing

.gitignore

@@ -40,3 +40,9 @@ playwright/tmp/
 playwright-report/
 test-results/
 tmp/
+
+# pytest-cov artifacts, written by the coverage run CI gates on
+.coverage
+.coverage.*
+coverage.xml
+htmlcov/

AGENTS.md

@@ -39,6 +39,8 @@ Version `0.2.4` · Python >= 3.12 · Apache 2.0
 ## Testing
 
 - Python unit tests: `pytest -m "not server"` (config in `pyproject.toml`, suite in `py/tests/`)
+- Fast loop: `pytest -m unit` — CI runs this as a gate, so mark every new file with a module-level
+  `pytestmark`; see `context/testing.md` for which marker and where the file goes
 - Start server (for Cypress): `visdom -port 8098 -env_path /tmp`
 - Baseline: `npm run test:init`
 - Run E2E tests: `npm run test`
@@ -47,6 +49,8 @@ Version `0.2.4` · Python >= 3.12 · Apache 2.0
 ## PR Checklist
 
 - Branch from `master`; add `pytest` tests in `py/tests/` for Python code and Cypress tests for frontend behavior
+- New test file: put it in `py/tests/unit/` or `py/tests/integration/`, and give it the matching
+  `pytestmark` — CI runs `-m unit` as a gate, so an unmarked file is in neither job
 - Update `README` for API changes, `__init__.pyi` for interface changes
 - Run linters, do not commit `py/visdom/static/`
 

example/manual/README.md

@@ -0,0 +1,31 @@
+<!-- Copyright 2017-present, The Visdom Authors
+     All rights reserved.
+
+     This source code is licensed under the license found in the
+     LICENSE file in the root directory of this source tree. -->
+
+# Manual checks
+
+Scripts here drive a **running** visdom server and are judged by looking at the
+browser. They are not tests: pytest does not collect them (`testpaths` is
+`py/tests`), and nothing in CI runs them.
+
+Automated checks belong elsewhere:
+
+| Question | Where it is answered |
+|---|---|
+| Does the server behave correctly? | `py/tests/` (pytest) |
+| Does the UI still render the same pixels? | `playwright/`, `cypress/` |
+| Does this *look* right to a person? | here |
+
+## `visual_check.py`
+
+Creates one window of each visualization type in the `visual_check` environment,
+then prints a checklist to walk through in the browser. Useful after a
+dependency bump or a change to the plotting payloads.
+
+```bash
+visdom -port 8097 -env_path /tmp     # in one shell
+python example/manual/visual_check.py
+# then open http://localhost:8097/env/visual_check
+```