test: add API integration suite, fixtures, and frontend unit tests#40
Open
clean6378-max-it wants to merge 5 commits into
Open
test: add API integration suite, fixtures, and frontend unit tests#40clean6378-max-it wants to merge 5 commits into
clean6378-max-it wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds search limit validation, Python integration tests and fixtures, Vitest frontend unit tests and config, and CI jobs to run both suites and upload coverage artifacts. ChangesComprehensive Test Suite
Sequence Diagram(s)sequenceDiagram
participant DevMachine
participant GitHubActions
participant TestClient
participant FlaskApp
participant JSRunner
DevMachine->>GitHubActions: push PR (CI triggers)
GitHubActions->>FlaskApp: run integration-tests (pytest -> coverage.xml)
GitHubActions->>TestClient: executes pytest using test clients (conftest)
TestClient->>FlaskApp: GET /api/search?q=...&limit=...
FlaskApp->>FlaskApp: _parse_limit -> validate/cap
FlaskApp-->>TestClient: 200/400 JSON
GitHubActions->>JSRunner: run js-tests (Node v20 + vitest)
JSRunner-->>GitHubActions: test results & coverage
GitHubActions->>GitHubActions: upload coverage-report artifact
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
static/js/shared/state.test.js (1)
13-17: ⚡ Quick winReset singleton state via hook instead of inline cleanup.
If this test fails before Line 17, shared state can leak into subsequent tests.
Proposed fix
-import { describe, it, expect } from 'vitest'; +import { describe, it, expect, beforeEach } from 'vitest'; import { state } from './state.js'; describe('state', () => { + beforeEach(() => { + state.currentProject = null; + state.cachedSessions = []; + }); + @@ it('allows updating currentProject', () => { state.currentProject = 'test-project'; expect(state.currentProject).toBe('test-project'); - state.currentProject = null; }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/js/shared/state.test.js` around lines 13 - 17, The test mutates the shared singleton `state` inline and cleans up only within the test, risking leaks if the test fails; replace the inline cleanup with a test lifecycle hook (e.g., `afterEach`) that resets the singleton so all tests always run with a clean state — specifically reset `state.currentProject` (or call an existing `state.reset()` method if present) in an `afterEach` hook rather than setting `state.currentProject = null` at the end of the `it('allows updating currentProject')` test.static/js/shared/markdown.test.js (1)
16-23: ⚡ Quick winRestore global mocks after each test to prevent cross-suite leakage.
This suite mutates
globalThis.marked/globalThis.DOMPurifybut never restores prior values, which can make other tests order-dependent.Proposed fix
import { describe, it, expect, vi, beforeEach } from 'vitest'; import { cleanContent, renderMarkdown } from './markdown.js'; describe('cleanContent', () => { @@ describe('renderMarkdown', () => { + let originalMarked; + let originalDOMPurify; + beforeEach(() => { + originalMarked = globalThis.marked; + originalDOMPurify = globalThis.DOMPurify; globalThis.marked = { parse: vi.fn((text) => `<p>${text}</p>`), }; globalThis.DOMPurify = { sanitize: vi.fn((html) => html.replace(/<script[\s\S]*?<\/script>/gi, '')), }; }); + + afterEach(() => { + globalThis.marked = originalMarked; + globalThis.DOMPurify = originalDOMPurify; + });Also applies to: 31-33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@static/js/shared/markdown.test.js` around lines 16 - 23, The test suite replaces globalThis.marked and globalThis.DOMPurify in beforeEach but never restores them, causing cross-test leakage; modify the tests to save the originals (e.g., const _origMarked = globalThis.marked, const _origDOMPurify = globalThis.DOMPurify) before mocking in beforeEach and add an afterEach that restores globalThis.marked = _origMarked and globalThis.DOMPurify = _origDOMPurify (and clears any vi mocks) so both the beforeEach block(s) that set parse and sanitize mocks are properly reverted after each test.tests/test_search.py (1)
43-51: ⚡ Quick winAdd explicit boundary tests for
limit < 1.Given this file is the limit-validation suite, add checks for
limit=0and negativelimitto lock the contract already implemented in the API.Proposed patch
def test_limit_default(client): resp = client.get("/api/search?q=Hello") assert resp.status_code == 200 +def test_limit_zero_rejected(client): + resp = client.get("/api/search?q=Hello&limit=0") + assert resp.status_code == 400 + assert "error" in resp.get_json() + + +def test_limit_negative_rejected(client): + resp = client.get("/api/search?q=Hello&limit=-5") + assert resp.status_code == 400 + assert "error" in resp.get_json() + + def test_empty_query(client): resp = client.get("/api/search?q=") assert resp.status_code == 200 assert resp.get_json() == []🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_search.py` around lines 43 - 51, Add two explicit tests in tests/test_search.py (e.g., test_limit_zero and test_limit_negative) that call the search endpoint with limit=0 and limit=-1 (e.g., client.get("/api/search?q=Hello&limit=0") and client.get("/api/search?q=Hello&limit=-1")) and assert the API rejects these boundaries by returning an error status (assert resp.status_code == 400) to lock the contract for limit < 1; place them alongside the existing test_limit_default and test_empty_query functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 65-67: Replace tag refs with immutable commit SHAs for all GitHub
Action usages that currently use tags (e.g., actions/checkout@v4 and
actions/setup-python@v5 and the other tag refs in the file) by finding the
corresponding action repositories (actions/checkout, actions/setup-python,
etc.), selecting a specific commit SHA from the release you want to track, and
updating each usage to the full commit SHA (e.g.,
actions/checkout@<commit-sha>). Ensure every occurrence of a tag ref in the
workflow is changed to its full commit SHA so the CI runs deterministically and
securely.
- Line 65: Update every checkout step that uses "uses: actions/checkout@v4" to
include the option persist-credentials: false so the runner won't expose the
GITHUB_TOKEN to later steps; specifically modify each checkout occurrence (the
ones using actions/checkout@v4) to add a "with:" block containing
"persist-credentials: false" ensuring proper YAML indentation under the checkout
step, and apply this change to all four occurrences referenced in the workflow.
- Around line 61-98: Add an explicit permissions block to each job
(integration-tests and js-tests) to avoid default token permissions: insert a
permissions: contents: read stanza under each job definition to restrict to
least privilege, and for the integration-tests job (which runs
actions/upload-artifact) change to permissions: contents: write only if the
artifact upload fails with insufficient rights—otherwise keep read; ensure the
permissions block is placed at the same indentation level as name/runs-on for
the jobs.
In `@api/search.py`:
- Around line 19-23: The current check treats only exact empty string and misses
whitespace-only values, causing inputs like " " to be treated as invalid;
update the limit-parsing logic (the branch around raw, the int conversion where
n = int(raw) and the except ValueError) to treat raw as blank when raw is None
or raw.strip() == "" and to call int(raw.strip()) when converting; this will
make whitespace-only input default to _DEFAULT_LIMIT and still raise ValueError
for non-numeric content.
---
Nitpick comments:
In `@static/js/shared/markdown.test.js`:
- Around line 16-23: The test suite replaces globalThis.marked and
globalThis.DOMPurify in beforeEach but never restores them, causing cross-test
leakage; modify the tests to save the originals (e.g., const _origMarked =
globalThis.marked, const _origDOMPurify = globalThis.DOMPurify) before mocking
in beforeEach and add an afterEach that restores globalThis.marked = _origMarked
and globalThis.DOMPurify = _origDOMPurify (and clears any vi mocks) so both the
beforeEach block(s) that set parse and sanitize mocks are properly reverted
after each test.
In `@static/js/shared/state.test.js`:
- Around line 13-17: The test mutates the shared singleton `state` inline and
cleans up only within the test, risking leaks if the test fails; replace the
inline cleanup with a test lifecycle hook (e.g., `afterEach`) that resets the
singleton so all tests always run with a clean state — specifically reset
`state.currentProject` (or call an existing `state.reset()` method if present)
in an `afterEach` hook rather than setting `state.currentProject = null` at the
end of the `it('allows updating currentProject')` test.
In `@tests/test_search.py`:
- Around line 43-51: Add two explicit tests in tests/test_search.py (e.g.,
test_limit_zero and test_limit_negative) that call the search endpoint with
limit=0 and limit=-1 (e.g., client.get("/api/search?q=Hello&limit=0") and
client.get("/api/search?q=Hello&limit=-1")) and assert the API rejects these
boundaries by returning an error status (assert resp.status_code == 400) to lock
the contract for limit < 1; place them alongside the existing test_limit_default
and test_empty_query functions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 486b1227-11e0-4657-8bf8-62f272633275
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
.coverage.github/workflows/ci.yml.gitignoreapi/search.pypackage.jsonpyproject.tomlrequirements-dev.txtstatic/js/shared/markdown.test.jsstatic/js/shared/state.test.jsstatic/js/shared/utils.test.jstests/fixtures/session_minimal.jsonltests/fixtures/session_with_thinking.jsonltests/fixtures/session_with_tools.jsonltests/test_api_integration.pytests/test_search.pyvitest.config.js
Extract Flask client fixtures to tests/conftest.py, use client_single for search limit tests, and relax session count assertion to >= 1. Include utils/ in coverage (omit untested export modules), stop tracking .coverage, and align integration-tests CI with full cov scope. Co-Authored-By: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_search.py (1)
27-35: ⚡ Quick winStrengthen default-limit assertions with response shape checks.
Line 29 and Line 34 only assert status code. Add list-shape assertions so these tests catch response-body regressions too.
✅ Suggested test hardening
def test_limit_default(client_single): resp = client_single.get("/api/search?q=Hello") assert resp.status_code == 200 + assert isinstance(resp.get_json(), list) def test_limit_whitespace_defaults(client_single): resp = client_single.get("/api/search?q=Hello&limit=%20%20%20") assert resp.status_code == 200 + assert isinstance(resp.get_json(), list)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_search.py` around lines 27 - 35, Update the two tests test_limit_default and test_limit_whitespace_defaults to validate the response body shape in addition to status 200: parse resp.json(), assert the top-level value is a list (e.g., isinstance(resp.json(), list) or len check) and optionally assert each item is a dict or contains expected keys (e.g., "id" or "text") to catch body regressions; keep the existing status_code assertions and only add these lightweight structural checks in both functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_search.py`:
- Around line 27-35: Update the two tests test_limit_default and
test_limit_whitespace_defaults to validate the response body shape in addition
to status 200: parse resp.json(), assert the top-level value is a list (e.g.,
isinstance(resp.json(), list) or len check) and optionally assert each item is a
dict or contains expected keys (e.g., "id" or "text") to catch body regressions;
keep the existing status_code assertions and only add these lightweight
structural checks in both functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ece97f9-45c4-4c60-82e9-08c1687b4aed
📒 Files selected for processing (7)
.coverage.github/workflows/ci.yml.gitignorepyproject.tomltests/conftest.pytests/test_api_integration.pytests/test_search.py
✅ Files skipped from review due to trivial changes (1)
- .gitignore
timon0305
requested changes
May 20, 2026
Comment on lines
+36
to
+39
| def test_projects_empty_base_dir(tmp_path): | ||
| app = create_app(base_dir=str(tmp_path)) | ||
| app.config["TESTING"] = True | ||
| resp = app.test_client().get("/api/projects") |
Collaborator
There was a problem hiding this comment.
This test bypasses the client fixture from conftest.py because it wants a tmp_path with no seeded sessions. Cleaner to add a third fixture client_empty (returning a client over an empty tmp_path) and use it here. Right now this test rebuilds the entire app setup inline.
Comment on lines
+17
to
+27
| def _parse_limit(raw: str | None) -> int: | ||
| """Parse positive integer limit query param; raise ValueError if invalid.""" | ||
| if raw is None or raw.strip() == "": | ||
| return _DEFAULT_LIMIT | ||
| try: | ||
| n = int(raw.strip()) | ||
| except ValueError: | ||
| raise ValueError("limit must be a positive integer") from None | ||
| if n < 1: | ||
| raise ValueError("limit must be a positive integer") | ||
| return min(n, _MAX_LIMIT) |
Collaborator
There was a problem hiding this comment.
from None suppresses the upstream chain. For server-side debugging, you lose the original int("1.5") message ("invalid literal for int() with base 10: '1.5'"). Suggest either dropping from None so the chain stays for tracebacks, or using from exc to keep the cause visible. The client-facing message stays the same either way (it's the outer ValueError's string).
Collaborator
There was a problem hiding this comment.
The thinking-block fixture is added but test_api_integration.py and test_search.py don't reference it (only session_minimal.jsonl and session_with_tools.jsonl are copied via the conftest.py fixtures). Either remove it or add a test that exercises the thinking content-block path through the session-detail endpoint.
Collaborator
There was a problem hiding this comment.
.coverage is checked in as a tracked binary file. The same PR adds .coverage to .gitignore on the line below, but gitignore doesn't retroactively untrack files that are already tracked — the binary will keep regenerating on every CI run and getting committed unless it's explicitly untracked first.
Add client_empty and client_thinking fixtures, exercise thinking blocks via session detail API, preserve ValueError cause in _parse_limit, and remove .coverage from git tracking (already in .gitignore). Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_api_integration.py`:
- Around line 83-85: The test currently asserts the thinking text only on
assistant_msgs[0], which is order-dependent; update the assertion to check that
any assistant message in assistant_msgs has get("thinking") == "Considering
options carefully." (e.g., replace the index-based check with an any(...) or
equivalent on assistant_msgs) so the test passes regardless of message order
while still verifying the presence of the expected thinking field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd357113-1c3c-4b58-a6af-97444da65d88
📒 Files selected for processing (3)
api/search.pytests/conftest.pytests/test_api_integration.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #39
Problem
static/js/shared/were verified only manually.int(limit)on unvalidated query params could 500 onlimit=abc.api/blueprints were not.Changes
Search limit validation (Monday prerequisite, included)
api/search.py:_parse_limit()validateslimit; invalid values → 400 +{"error": "..."}.< 1.tests/test_search.py: 5 tests (integer string, float, non-numeric, default, empty query).Fixtures
tests/fixtures/session_minimal.jsonl— titled session with searchable "Hello" texttests/fixtures/session_with_tools.jsonl— tool_use + tool result pathtests/fixtures/session_with_thinking.jsonl— thinking block pathPython API integration tests
New
tests/test_api_integration.py— 10 scenarios:GET /api/projects[]GET /api/projects/<name>/sessions[]GET /api/sessions/<name>/<id>errorkeyGET /api/searchq;limit=abc→ 400; validlimit=5Frontend JS tests (vitest)
package.json,package-lock.json,vitest.config.js(jsdom)static/js/shared/utils.test.js— 7 tests (esc,truncate,formatTs,formatDate,formatSize)static/js/shared/markdown.test.js— 4 tests (cleanContent, DOMPurify sanitization, fallback)static/js/shared/state.test.js— 3 tests (sharedstateobject)Coverage + CI
requirements-dev.txt:pytest-cov>=5.0pyproject.toml:--cov=api --cov=utils, 60% fail-under onapi/.github/workflows/ci.yml:integration-tests+js-testsjobs.gitignore:node_modules/,coverage/,coverage.xmlOut of scope
ErrorCodeon error responses (Wednesday)CONTRIBUTING.md(Thursday)models/TypedDict package (Monday PR — separate branch if not merged)Test plan
pytest tests/test_search.py tests/test_api_integration.py -v— 15 passedpytest -q— 244 passedapi/)npm test— 14 vitest tests passedintegration-testsjob greenjs-testsjob greenpytestjob green (full suite with coverage addopts)