refactor: consolidate test infrastructure and document conventions by NetDevAutomate · Pull Request #1 · NetDevAutomate/StudyLoop

NetDevAutomate · 2026-04-01T14:14:59Z

Summary

Dedup migrated_db fixture — 7 identical copies across exporter test files consolidated into conftest.py
Replace unused pytest.mark.live with integration marker declared at workspace root
Add studyctl pytest config — [tool.pytest.ini_options] with markers and --tb=short
Create _helpers.py — shared fixture factories for studyctl tests (can't use conftest due to pluggy conflict)
Write docs/TESTING.md — complete testing conventions guide covering fixtures, mocking, markers, and the conftest prohibition

Test plan

Full test suite: 650 passed, 5 skipped (identical to baseline before changes)
migrated_db only defined in conftest, removed from all 7 exporter test files
pytest.mark.integration visible from workspace root (uv run pytest --markers)
_helpers.py imports cleanly and is not collected by pytest
All pre-commit hooks pass (ruff, ruff-format, pyright, secrets)

Post-Deploy Monitoring & Validation

No additional operational monitoring required: pure test infrastructure refactor with zero behavioural changes to application code.

🤖 Generated with Claude Code

Dedup migrated_db fixture (7 copies → 1 in conftest), replace unused pytest.mark.live with integration marker, add studyctl pytest config, create _helpers.py for shared fixture factories, and write TESTING.md covering all conventions. 650 tests pass unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Refactors and consolidates pytest infrastructure across the workspace to reduce fixture duplication, standardize markers/config, and document testing conventions for contributors.

Changes:

Consolidates duplicated migrated_db fixtures into packages/agent-session-tools/tests/conftest.py.
Standardizes on an integration pytest marker and declares it in root + package configs.
Adds studyctl-specific test helpers and introduces docs/TESTING.md documenting conventions and invocation patterns.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`pyproject.toml`	Declares the `integration` marker at workspace root for root-level pytest runs.
`packages/studyctl/tests/_helpers.py`	Adds shared factory helpers intended to replace the need for a studyctl `conftest.py`.
`packages/studyctl/pyproject.toml`	Adds pytest config for studyctl when running tests from within the package directory.
`packages/agent-session-tools/tests/test_exporter_*.py`	Removes duplicated `migrated_db` fixtures/imports now provided via conftest.
`packages/agent-session-tools/tests/conftest.py`	Introduces the centralized `migrated_db` fixture (runs migrations on `temp_db`).
`packages/agent-session-tools/pyproject.toml`	Replaces the unused `live` marker with the standardized `integration` marker.
`docs/TESTING.md`	Adds comprehensive testing conventions and how-to-run guidance.
`docs/brainstorms/2026-04-01-test-consolidation-tdd-foundation-brainstorm.md`	Adds a decision record/brainstorm capturing rationale and testing strategy.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-01T14:22:12Z

+Usage::
+
+    from _helpers import make_review_db, make_isolated_config
+
+    @pytest.fixture()
+    def review_db(tmp_path):
+        return make_review_db(tmp_path)
+
+    @pytest.fixture(autouse=True)
+    def isolated_config(tmp_path, monkeypatch):
+        return make_isolated_config(tmp_path, monkeypatch)


The usage example imports helpers via from _helpers import ..., but _helpers.py lives under packages/studyctl/tests/ and neither the workspace-root nor packages/studyctl pytest config sets pythonpath to include that directory. In typical pytest runs this makes _helpers not importable (you'd need to either add an explicit pythonpath entry or move/rename the helpers into an importable module under studyctl so imports are stable from both root and package invocations).

Copilot · 2026-04-01T14:22:13Z

+    db_path = tmp_path / "reviews.db"
+    # Create the file so ensure_tables finds it
+    conn = sqlite3.connect(str(db_path))
+    conn.execute("PRAGMA journal_mode=WAL")
+    conn.execute("PRAGMA busy_timeout=5000")
+    conn.close()
+
+    # Import lazily so the module can be loaded even if studyctl
+    # isn't fully installed (e.g. during collection).
+    from studyctl.review_db import ensure_tables
+
+    ensure_tables(db_path)


make_review_db() manually opens/closes a sqlite connection just to create the file and set WAL/busy_timeout, but studyctl.review_db.ensure_tables() already uses _connect() which sets those pragmas. Consider simplifying this helper to just ensure the file exists and then rely on ensure_tables() for connection setup, to avoid duplicated DB setup logic that can drift from production behavior.

Copilot · 2026-04-01T14:22:13Z

+    """Redirect studyctl's central config paths to a temp directory.
+
+    Patches ``studyctl.settings.CONFIG_DIR`` and
+    ``studyctl.settings._CONFIG_PATH`` so all config-reading code
+    hits *tmp_path* instead of ``~/.config/studyctl``.
+


The docstring for make_isolated_config() says it redirects config paths so “all config-reading code” hits tmp_path, but some modules import CONFIG_DIR at module import time (e.g. studyctl.cli._setup.CONFIG_DIR) and won’t be affected by patching studyctl.settings.CONFIG_DIR. Either narrow the claim in the docstring or extend the helper to also patch those known import sites to prevent tests from accidentally writing to the user’s real config directory.

Copilot · 2026-04-01T14:22:13Z

+The workspace has two packages with independent test suites:
+
+| Package | Tests | Path |
+|---------|-------|------|
+| studyctl | 293 | `packages/studyctl/tests/` |
+| agent-session-tools | 357 | `packages/agent-session-tools/tests/` |
+


This guide hard-codes exact test counts (293/357/650). Those numbers will drift as tests are added/removed, which can quickly make the doc misleading. Consider removing the counts or phrasing them as approximate (or pointing readers to pytest --collect-only/pytest -q to see current totals).

Copilot · 2026-04-01T14:22:14Z

+Import factory functions from `_helpers.py` and wrap them in `@pytest.fixture`:
+
+```python
+import pytest
+from _helpers import make_review_db, make_isolated_config
+
+
+@pytest.fixture()
+def review_db(tmp_path):
+    return make_review_db(tmp_path)
+


The _helpers.py import example uses from _helpers import ..., but _helpers.py is nested under packages/studyctl/tests/ and the documented root invocation uses --import-mode=importlib without adding that directory to pythonpath. As written, this import is likely to fail; consider updating the recommended import path and/or pytest config so helper imports work consistently from both workspace-root and per-package test runs.

Brings the Playwright suite from 10 passing / 3 broken to 126 passing / 4 intentionally skipped. All surfaces of the Phase 1 web UI are now covered end-to-end. New test files -------------- - test_web_review_flow.py (13 tests) — reviewApp() across flashcards and quiz modes: courses view, card rendering, config navigation, start-session flow, flip card, correct/wrong counters, retry-wrong cycle, quiz lock-after-answer, summary view + percentage calc. Uses Playwright route interception to stub /api/courses, /api/cards, /api/sources, /api/stats, /api/review — no real content needed on disk. - test_web_session_lifecycle.py (12 tests) — sessionTimer() picker + full start flow: agent hydration from /session/options, target-kind switcher parametrised over all 4 kinds, start-button enable/disable, study-session-start event dispatch on 201, 503 install_hint surfacing (the §1.5b structured error), 409 already-active UX, network-failure UX, end-session confirm dialog. - test_web_stores.py (13 tests) — Alpine ``settings`` + ``pomodoro`` stores: theme toggle + localStorage persistence, dyslexic font toggle, voice toggle, stopSpeaking idempotency, pomodoro start / pause / stop lifecycle, saveDurations persistence + recomputation, header-button wiring. - test_web_agent_matrix.py (15 tests) — per-agent parametrised (claude, codex, gemini, kiro, opencode × 3 tests each). Spawns the server with STUDYLOOP_TEST_AGENT_CMD='echo agent-stub-ready; exec cat' so every agent's launch path is driven uniformly without needing the real binary. Asserts: POST /session/start returns 201 + ws_url with the correct agent, WebSocket emits Started(agent=X), binary output frames stream the stub banner through the pump. This is the Amendment #1 "each coding agent with a test prompt that returns the expected response" requirement, literally. Supporting: _playwright_helpers.py (already landed in the nav commit) ------------------------------------------------------------------ - web_server_fixture_factory(port), auth_context_fixture_factory(), web_page_fixture_factory(server, auth) — shared factories so test files don't copy-paste server boot + HTTP Basic Auth wiring. - clean_ipc() + effective_credentials() — utilities reused by the per-agent matrix fixture which needs its own Popen to inject env. Deleted: test_e2e_session_demo.py ---------------------------------- Pre-existing recorded-demo integration test whose assertions targeted the pre-§1.7 #study-session UI (.meta-topic / .meta-energy / etc., all gone after the xterm.js rewrite). Replaced by the focused tests above which cover the same surface with much less coupling and run ten times faster. User-approved deletion. Port allocation (documented here so future tests don't collide) ---------------------------------------------------------------- 18570 — test_web_navigation 18571 — test_web_review_flow 18572 — test_web_session_lifecycle 18573 — test_web_stores 18574 — test_web_agent_matrix 18567 — test_web_terminal (shared with 18568, 18569 — pre-existing) Final state ----------- Main suite: 2001 passed, 210 deselected (the e2e set). E2e suite: 126 passed, 4 skipped (intentional — all documented), 0 failed. Skip reasons: xterm.js canvas input unreliable in headless Chromium (3 tests), and the collapse-toggle test that needs a real ttyd (1 test, correctly flagged since Phase 0). Ruff + format clean. Plan: Amendment #1 "comprehensive Playwright test suite as a first-class deliverable" — status: landed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Completes the original feature: a user who selects a provider with no stored key can now enter it in the UI. Previously the backend secrets routes existed and were tested, but the frontend never called them — there was no way to add a key through the web app. - index.html: inline key-entry row, shown only when the selected provider is keyed (adapter != bedrock) AND not yet available. Password input + 'Test & save' -> POST /api/content/secrets; on success clears the raw key from memory, re-fetches /providers so the option flips to enabled, shows a 'verified & stored encrypted' confirmation; on 400/422 shows the provider's rejection message. needsKey / selectedProvider getters; keyEntry state reset on provider change. - style.css: minimal feedback styles (key-error/key-ok/key-hint) using existing palette vars; password/text inputs styled to match selects. - test_web_key_entry_e2e.py: 4 real-browser e2e (port 18582) — form shows for unavailable keyed provider, hidden for available one, save POSTs correct {provider,key} body + shows success, rejection shows error. Gate: 4/4 new e2e + 10/10 existing content-gen e2e green. With #1/#3 (4f1425c) the feature now works end-to-end: select provider -> enter missing key -> tested + stored encrypted -> consumed at generation.

Generated decks never appeared in the Quiz/Flashcard panels. Root cause was TWO independent bugs, not the single "3-level layout" hypothesis in the handoff: A. Read-root unconfigured (dominant). The panels read decks from `review.directories`; the generator WRITES them under `content.base_path`. These are two separate config keys that must agree, but the live config sets only `content`, so `_web.py` handed `discover_directories([])` and the panels showed nothing regardless of disk contents. Fix: new `settings.resolve_study_dirs()` falls back to `content.base_path` when `review.directories` is unset. Wired into `_web.py` and `mcp/tools.py`. B. Single-level descent. The real vault is 3 levels deep (`base/<publisher>/<course>/{flashcards,quizzes}/`) but `discover_directories` only walked one child level (the publisher), never reaching course dirs. Fix: recursive descent to depth 4, stopping at the first content-bearing dir on each branch (a course is a leaf — never recurse into its deck subdirs, and skip empty deck dirs that `get_course_dir` eagerly mkdirs). Proven end-to-end (scripts/prove_gen_readpath.py + live server + Playwright): generated a real Ollama/gemma4 deck into CodeWithMosh/Complete_SQL_Mastery, confirmed files land where panels read, `/api/courses` lists it (9 fc / 6 quiz), `/api/cards` returns quality content, and the Flashcard panel renders card 1/9. Tests: +9 (4 loader, 4 resolver, 1 empty-dir guard). 87 passed, no regressions.

…tion - scripts/prove_gen_readpath.py: end-to-end proof that a generated deck lands where the panels read (drives job.run_job + settings.resolve_study_dirs + review_loader.discover_directories + services.review.get_cards). Used to prove Issue #1's fix. - scripts/gen_for_workflow.py: parameterised single-provider generation harness for the autonomous gen+judge workflow. Controls target card count and injects judge-feedback guidance into the prompts (generators have no count param); decouples SOURCE course (scope) from OUTPUT course (--output-course) so concurrent providers write to isolated dirs; emits WF_RESULT_JSON for the orchestrating workflow to parse. Both proven live against Ollama gemma4, Bedrock Sonnet 4.6, and MiniMax M2.7.

…omplete Final overnight handoff + the autonomous workflow's per-provider report. Outcome: - Issue #1 (decks invisible to panels) FIXED & PROVEN — was 2 bugs (read-root unconfigured + 1-level-only discovery), not the hypothesised single 3-level one. - Autonomous gen+judge+validate+report workflow built, launched, completed: Bedrock APPROVED 9/9, Ollama APPROVED 8/7, MiniMax generates cleanly (2 adapter bugs fixed) but quiz quality is below the Opus judge bar (model limitation). - Playwright validated all 3 provider courses render in the panels. - Issue #2 (6 e2e selector failures) fixed. Issue #3 (model dropdown) deferred with rationale. Open product decision for the user: MiniMax rotation (flashcards-only / both / drop-for-quizzes). No further code needed for MiniMax to function.

…es, scalable course list Brings 20 commits to main: - Settings → LLM Providers admin panel (auth-kind taxonomy, Bedrock bearer, Ollama URL, per-provider test) + e2e selector fixes. - Content-gen fixes: Issue #1 deck write-root/read-root reconciliation (resolve_study_dirs + recursive discovery); MiniMax tool_result correction, inline-XML tool-call fallback, transient-retry; --max-retries harness knob. - Autonomous generate+judge+validate+report workflow (3 providers). - Scalable course list: publisher field, mode-split Flashcards/Quizzes panels, collapsible publisher groups, search, compact rows (+37 e2e tests).

…-side ACP guard (P0) Audit P0 #1 + #2. The user-controlled topic became a session-dir path segment via a naive .replace(' ','-') slug in FOUR places, none of which stripped '/', '\\' or '..' — and the dir is later rmtree'd on failure, so a topic like '../../x' was a real escape+delete vector. - New slug_session_dir() collapses everything outside [a-z0-9] to '-' and falls back to 'session' when empty; session_dir_name() routes through it. - Replace the three inline duplicate slugs (web PTY/ttyd _start.py, CLI session/start.py) with the shared helper — root-cause fix, one segment. - Add server-side ACP capability guard in _start_acp_session: a PTY-only agent (Claude Code, Codex) requesting transport=acp now gets a 400 with cause + repair BEFORE any spawn, instead of an opaque failure. - ACP_CAPABLE_AGENTS is the single source of truth; _options.py's picker flags now reference it instead of a duplicated {kiro,gemini,grok} literal. Tests: parametrized traversal cases + empty-slug fallback + ACP-guard rejection for claude/codex + capable-set lock. Held-out gate: the traversal tests fail on the pre-fix slug (verified by revert), pass on the fix.

Council item C6 (SIGNOFF-M2/ARBITRATION.md), matrix gap #1 of 3 (the other two -- C1's interleaving, C4's restart-with-live-child -- are closed by those items' own tests). No code change: claim_blocks_cli_start never consults _grace state, only pid liveness, so this cell was already correct by construction. The test exists to pin that the two mechanisms compose correctly, not to fix a defect. Test: TestWebThenCli:: test_cli_start_is_refused_while_a_web_session_is_detached_within_grace -- schedules a real grace-release timer for a web claim with a live pid, confirms has_pending_release() is true, then asserts a CLI start still refuses with the existing message and logs no reclaim. Evidence: reviews/2026-09-02-full-repo-review/evidence/M2/step-9/C6/. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>