feat: add CI pipeline, pre-commit hooks, Biome config, and test suite#654
Open
neuron-tech-ai wants to merge 2 commits into
Open
feat: add CI pipeline, pre-commit hooks, Biome config, and test suite#654neuron-tech-ai wants to merge 2 commits into
neuron-tech-ai wants to merge 2 commits into
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR establishes comprehensive testing infrastructure for the voicebox project by adding Vitest for frontend tests, expanding backend test coverage with fixtures and multiple test modules, implementing automated CI/CD workflows, and providing cross-platform developer environment setup tools—enabling consistent testing, code quality gates, and reproducible development across the team. ChangesTesting Infrastructure and Development Workflows
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/tests/test_all_models_e2e.py (1)
180-205:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid returning before cleanup in
ServerProcess.stop().At Line 183-Line 184,
stop()returns immediately if the child already exited, which skips reader-thread join and log-file close. That can truncate logs and leak file handles.Suggested fix
def stop(self) -> None: if self.proc is None: return - if self.proc.poll() is not None: - return - try: - if platform.system() == "Windows": - subprocess.run( - ["taskkill", "/F", "/T", "/PID", str(self.proc.pid)], - capture_output=True, - ) - else: - self.proc.send_signal(signal.SIGTERM) - except Exception as e: - print(f"[shutdown] signal failed: {e}", flush=True) - try: - self.proc.wait(timeout=10) - except subprocess.TimeoutExpired: - print("[shutdown] server didn't exit cleanly, killing", flush=True) - self.proc.kill() - with contextlib.suppress(subprocess.TimeoutExpired): - self.proc.wait(timeout=5) + if self.proc.poll() is None: + try: + if platform.system() == "Windows": + subprocess.run( + ["taskkill", "/F", "/T", "/PID", str(self.proc.pid)], + capture_output=True, + ) + else: + self.proc.send_signal(signal.SIGTERM) + except Exception as e: + print(f"[shutdown] signal failed: {e}", flush=True) + try: + self.proc.wait(timeout=10) + except subprocess.TimeoutExpired: + print("[shutdown] server didn't exit cleanly, killing", flush=True) + self.proc.kill() + with contextlib.suppress(subprocess.TimeoutExpired): + self.proc.wait(timeout=5) if self._reader_thread is not None: self._reader_thread.join(timeout=2) with contextlib.suppress(Exception): self._log_fh.close()🤖 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 `@backend/tests/test_all_models_e2e.py` around lines 180 - 205, The stop method in ServerProcess.stop currently returns early when self.proc.poll() is not None, skipping reader-thread join and log-file close; change the flow so you only return immediately if self.proc is None, but if self.proc has already exited (self.proc.poll() is not None) proceed to join self._reader_thread (if set) and close self._log_fh (with contextlib.suppress) instead of returning; only run the shutdown/kill logic (taskkill/send_signal/wait/kill) when the process is still running. Ensure references to self.proc, self._reader_thread, and self._log_fh in the method are preserved and reused in the revised control flow.CONTRIBUTING.md (1)
274-280:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPython formatter guidance is outdated.
Line 279 still points to Black, but this tooling setup enforces Ruff formatting. Aligning this avoids contributor confusion.
Suggested patch
- Follow PEP 8 style guide - Use type hints - Use async/await for I/O operations -- Format with Black (if configured) +- Format with Ruff (`ruff format`)🤖 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 `@CONTRIBUTING.md` around lines 274 - 280, Update the Python formatter guidance in the "Python" section to remove the outdated reference to Black and replace it with the current enforced formatter (Ruff), i.e., edit the bullet that says "Format with Black (if configured)" to mention Ruff (and any config/tooling notes) so contributors know to use Ruff for formatting and linting; update nearby wording if needed to reflect Ruff's usage alongside Black's predecessor..pre-commit-config.yaml (1)
30-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPre-commit is missing Python typecheck parity.
If local gates are meant to mirror CI quality checks, add a Pyright hook; otherwise Python type regressions can pass local hooks.
Suggested hook addition
- repo: local hooks: + - id: pyright + name: Python typecheck (pyright) + language: system + entry: bash -c 'cd backend && pyright' + pass_filenames: false + types: [python] + - id: biome-check name: Biome lint + format language: system entry: bun run check:fix🤖 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 @.pre-commit-config.yaml around lines 30 - 73, Add a Pyright local hook to the existing "Tests" local repo hooks to enforce Python typechecks locally: create a new hook with id "pyright" (name like "Python typecheck (pyright)"), language "system", entry that runs pyright in the backend (e.g., change directory to backend and run the pyright CLI), set pass_filenames: false, types or files to target Python files (e.g., types: [python] or files: \.py$), and include it alongside the existing pytest-unit/pytest-integration hooks (optionally as a pre-commit stage) so local pre-commit mirrors the CI typecheck.
🧹 Nitpick comments (4)
backend/tests/test_history.py (2)
30-30: ⚡ Quick winRemove unused
tmp_pathparameter.The fixture uses an in-memory SQLite database and does not write to
tmp_path. Remove the parameter to clarify the fixture's requirements.♻️ Proposed fix
`@pytest.fixture` -def db(tmp_path): +def db(): """In-memory SQLite session pre-loaded with a profile and some generations."""🤖 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 `@backend/tests/test_history.py` at line 30, The fixture function db in backend/tests/test_history.py currently declares an unused tmp_path parameter; remove tmp_path from the db fixture signature (def db(...)) so the fixture uses only the in-memory SQLite setup it needs, and adjust any references if they explicitly pass tmp_path (none expected) so tests continue to request the db fixture without the unused parameter.
86-90: ⚡ Quick winStrengthen the assertion to be deterministic.
The test knows exactly which row matches: only
"Hello world"(g1) contains "hello" case-insensitively. Useassert result.total == 1instead of>= 1for a more precise test.♻️ Proposed fix
def test_case_insensitive_via_like(self, db): # SQLite LIKE is case-insensitive for ASCII by default. result = _list_generations(HistoryQuery(search="hello"), db) - assert result.total >= 1 + assert result.total == 1 + assert result.items[0].id == "g1"🤖 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 `@backend/tests/test_history.py` around lines 86 - 90, Update the test_case_insensitive_via_like test to make the assertion deterministic: replace the loose assertion "assert result.total >= 1" with a strict equality "assert result.total == 1" so the test verifies that only the known row (the "Hello world" generation returned by _list_generations invoked with HistoryQuery(search=\"hello\")) matches; locate the assertion in test_case_insensitive_via_like and change the comparator accordingly.backend/tests/integration/test_history_api.py (1)
156-165: Clarify what the test actually verifies.The docstring states "this test additionally confirms that adding GenerationVersion rows doesn't change the total query count" but the test doesn't actually measure query count. It only verifies that version fields are populated correctly. Consider updating the docstring to accurately reflect what is tested:
📝 Suggested clarification
def test_batch_version_load_no_queries_per_item(self, seeded_db): - """Versions for all items are loaded in one query, not N queries. + """Versions are populated for all items without N+1 behavior. - We test this indirectly: if the N+1 is re-introduced the JOIN would - fail for items whose versions are loaded one-by-one and the profile_name - field would be empty or None. The profile_name assertion in - test_profile_name_populated_for_every_item already catches that; - this test additionally confirms that adding GenerationVersion rows - doesn't change the total query count from the caller's perspective. + If N+1 queries were re-introduced, version data might not be populated + correctly. This test verifies that after adding GenerationVersion rows, + the versions are correctly loaded and attached to their parent generations. """🤖 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 `@backend/tests/integration/test_history_api.py` around lines 156 - 165, Update the docstring for test_batch_version_load_no_queries_per_item to accurately describe its assertions: state that the test verifies that version-related fields (e.g., profile_name) are populated for every item even when GenerationVersion rows exist and that adding those rows does not break the JOIN behavior, rather than claiming it measures or confirms the total SQL query count; reference the test name test_profile_name_populated_for_every_item and the GenerationVersion rows in the description so readers understand the relationship between these tests.backend/tests/unit/test_history_search.py (1)
30-30: ⚡ Quick winRemove unused
tmp_pathparameter.The fixture creates an in-memory SQLite database (
"sqlite:///:memory:") and never usestmp_path. Remove the parameter to keep the signature clean.♻️ Proposed fix
`@pytest.fixture` -def db(tmp_path): +def db(): """In-memory SQLite session pre-loaded with a profile and test rows."""🤖 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 `@backend/tests/unit/test_history_search.py` at line 30, The fixture function db currently accepts an unused tmp_path parameter; update the fixture signature by removing the tmp_path parameter from the db fixture definition (the function named db) and any corresponding references in its usage so it only creates the in-memory SQLite engine ("sqlite:///:memory:") and returns the session/engine as before; ensure any tests or conftest references that call db as a fixture do not expect tmp_path to be provided.
🤖 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 73-108: The backend CI job lacks a Python typecheck step; add a
new step named "Typecheck (pyright)" in the backend-ci job (near the existing
"Lint (ruff)" and "Run tests (pytest)" steps) that installs pyright (e.g., npm
install -g pyright or pip install pyright) and runs the type checker in the
backend working-directory (run: pyright --project . or simply pyright) so static
typing is enforced as part of CI.
In `@app/src/__tests__/audio.test.ts`:
- Around line 19-24: The test title and assertion disagree: the test calls
createAudioUrl('xyz', 'http://localhost:17493/') and expects a double slash, so
rename the test description to match that asserted behavior (e.g., "does
double-slash when serverUrl has a trailing slash") or alternatively change the
expectation to the non-double-slash URL; locate the test in
app/src/__tests__/audio.test.ts around the it(...) that references
createAudioUrl and update the string title to reflect the actual expectation.
In `@Makefile`:
- Around line 63-66: The `install-system-linux` Makefile target may call `cargo`
immediately after running rustup, but the new cargo bin directory (~/.cargo/bin)
isn't in PATH until the current shell sources the environment, so the `cargo
install just` line can fail; update the target so that after running rustup you
source the cargo env (e.g. . "$HOME/.cargo/env" or source "$HOME/.cargo/env") in
the same shell session before invoking `cargo install just` (or prepend
PATH="$HOME/.cargo/bin:$PATH" for that command), ensuring the `cargo install
just` invocation in the Makefile uses the environment change in the same shell.
---
Outside diff comments:
In @.pre-commit-config.yaml:
- Around line 30-73: Add a Pyright local hook to the existing "Tests" local repo
hooks to enforce Python typechecks locally: create a new hook with id "pyright"
(name like "Python typecheck (pyright)"), language "system", entry that runs
pyright in the backend (e.g., change directory to backend and run the pyright
CLI), set pass_filenames: false, types or files to target Python files (e.g.,
types: [python] or files: \.py$), and include it alongside the existing
pytest-unit/pytest-integration hooks (optionally as a pre-commit stage) so local
pre-commit mirrors the CI typecheck.
In `@backend/tests/test_all_models_e2e.py`:
- Around line 180-205: The stop method in ServerProcess.stop currently returns
early when self.proc.poll() is not None, skipping reader-thread join and
log-file close; change the flow so you only return immediately if self.proc is
None, but if self.proc has already exited (self.proc.poll() is not None) proceed
to join self._reader_thread (if set) and close self._log_fh (with
contextlib.suppress) instead of returning; only run the shutdown/kill logic
(taskkill/send_signal/wait/kill) when the process is still running. Ensure
references to self.proc, self._reader_thread, and self._log_fh in the method are
preserved and reused in the revised control flow.
In `@CONTRIBUTING.md`:
- Around line 274-280: Update the Python formatter guidance in the "Python"
section to remove the outdated reference to Black and replace it with the
current enforced formatter (Ruff), i.e., edit the bullet that says "Format with
Black (if configured)" to mention Ruff (and any config/tooling notes) so
contributors know to use Ruff for formatting and linting; update nearby wording
if needed to reflect Ruff's usage alongside Black's predecessor.
---
Nitpick comments:
In `@backend/tests/integration/test_history_api.py`:
- Around line 156-165: Update the docstring for
test_batch_version_load_no_queries_per_item to accurately describe its
assertions: state that the test verifies that version-related fields (e.g.,
profile_name) are populated for every item even when GenerationVersion rows
exist and that adding those rows does not break the JOIN behavior, rather than
claiming it measures or confirms the total SQL query count; reference the test
name test_profile_name_populated_for_every_item and the GenerationVersion rows
in the description so readers understand the relationship between these tests.
In `@backend/tests/test_history.py`:
- Line 30: The fixture function db in backend/tests/test_history.py currently
declares an unused tmp_path parameter; remove tmp_path from the db fixture
signature (def db(...)) so the fixture uses only the in-memory SQLite setup it
needs, and adjust any references if they explicitly pass tmp_path (none
expected) so tests continue to request the db fixture without the unused
parameter.
- Around line 86-90: Update the test_case_insensitive_via_like test to make the
assertion deterministic: replace the loose assertion "assert result.total >= 1"
with a strict equality "assert result.total == 1" so the test verifies that only
the known row (the "Hello world" generation returned by _list_generations
invoked with HistoryQuery(search=\"hello\")) matches; locate the assertion in
test_case_insensitive_via_like and change the comparator accordingly.
In `@backend/tests/unit/test_history_search.py`:
- Line 30: The fixture function db currently accepts an unused tmp_path
parameter; update the fixture signature by removing the tmp_path parameter from
the db fixture definition (the function named db) and any corresponding
references in its usage so it only creates the in-memory SQLite engine
("sqlite:///:memory:") and returns the session/engine as before; ensure any
tests or conftest references that call db as a fixture do not expect tmp_path to
be provided.
🪄 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: 3db625be-0c04-4229-addf-8b996b3433f6
📒 Files selected for processing (34)
.github/workflows/ci.yml.pre-commit-config.yamlBrewfileCONTRIBUTING.mdMakefileapp/src/__tests__/audio.test.tsapp/src/__tests__/format.test.tsbackend/pyproject.tomlbackend/tests/conftest.pybackend/tests/integration/__init__.pybackend/tests/integration/conftest.pybackend/tests/integration/test_history_api.pybackend/tests/test_all_models_e2e.pybackend/tests/test_audio_preprocess.pybackend/tests/test_cors.pybackend/tests/test_generation_download.pybackend/tests/test_history.pybackend/tests/test_offline_guard.pybackend/tests/test_offline_patch.pybackend/tests/test_personality_samples.pybackend/tests/test_profile_duplicate_names.pybackend/tests/test_progress.pybackend/tests/test_qwen_download.pybackend/tests/test_refinement_collapse.pybackend/tests/test_refinement_samples.pybackend/tests/test_uploads.pybackend/tests/test_whisper_download.pybackend/tests/unit/__init__.pybackend/tests/unit/test_history_search.pybackend/tests/unit/test_upload_limits.pybiome.jsonjustfilepackage.jsonvitest.config.ts
💤 Files with no reviewable changes (1)
- backend/tests/test_refinement_collapse.py
- Add pyright typecheck step to backend-ci job - Ensure $HOME/.cargo/bin is on PATH before invoking cargo after rustup install - Rename misleading test case description to match asserted double-slash behavior
Author
|
Addressed all three CodeRabbit review comments:
|
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
1 participant
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.
Sets up the foundational development infrastructure that was missing:
The goal is to make it hard to accidentally merge broken or inconsistently formatted code.
Summary by CodeRabbit
New Features
Documentation
Chores