Skip to content

fix(test): drop unused appendFile import — green Tests badge - #31

Merged
omerakben merged 2 commits into
mainfrom
fix/v0.20.2-ci-typecheck-unused-import
May 14, 2026
Merged

fix(test): drop unused appendFile import — green Tests badge#31
omerakben merged 2 commits into
mainfrom
fix/v0.20.2-ci-typecheck-unused-import

Conversation

@omerakben

@omerakben omerakben commented May 14, 2026

Copy link
Copy Markdown
Owner

Summary

Two bugs were causing the README's Tests | failing red badge on main. This PR closes both:

  1. Commit 1 — typecheck failure: tests/demo/failure-gates.test.ts:24 imported appendFile but never used it. tsc --noEmit fails with TS6133 in CI, masking the second bug below.
  2. Commit 2 — root bun test discovery picked up GUI sub-package: code-oz-gui/tests/unit/*.test.ts requires @google/genai, which is a dep of the GUI sub-package — not the root. CI's bun install --frozen-lockfile only installs root deps, so bun test errored with "Cannot find module '@google/genai'" before the suite finished. Fix: scope root bun test to ./tests in both the workflow and the npm scripts. GUI unit tests remain runnable from inside code-oz-gui/ via its own runner.

Combined effect: green Tests + Release badges in the README hero, which is the visible signal that matters for first-time visitors. Per the v0.20.1 polish push, a red CI badge on a project literally about gating is the biggest credibility hit on the front page.

Test plan

  • bun run typecheck — was failing locally, now passes
  • bun run test — 3390 pass / 2 skip / 0 fail (./tests scope; GUI sub-package excluded)
  • CI test workflow goes green on this PR (both ubuntu-latest and macos-latest jobs)
  • After merge to main: Tests badge in README turns green
  • Static badge count updated 3395 → 3390 in README to match the scoped count

Refs: TD-5 in docs/handoffs/2026-05-14-v0.20.1-tech-debt.md (smoke script test-count badge comparison) is the upstream class of defect; both bugs slipped past pre-tag because the fresh-clone smoke didn't run bun test against a true clean clone with only root deps installed.

CI's typecheck step (tsc --noEmit, runs before bun test) failed on
TS6133 'appendFile' is declared but its value is never read'. The
failure-gates demo test never used the import. Removing it restores
the green Tests badge in the README hero without touching test logic.

Test count unchanged: 3395 pass / 2 skip / 0 fail.

Refs: TD-5 in docs/handoffs/2026-05-14-v0.20.1-tech-debt.md (smoke
script test-count badge comparison) — this is the upstream defect TD-5
was supposed to catch pre-tag.
Copilot AI review requested due to automatic review settings May 14, 2026 17:05
@coderabbitai

coderabbitai Bot commented May 14, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

The pull request removes an unused appendFile import from the node:fs/promises import declaration in the failure-gates test file, leaving only mkdtemp, mkdir, rm, and writeFile.

Changes

Test import cleanup

Layer / File(s) Summary
Remove unused appendFile import
tests/demo/failure-gates.test.ts
The appendFile function is removed from the node:fs/promises import list on line 24, eliminating an unused dependency.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Poem

🐰 One import unused, now gone with care,
Filesystem calls trim the bloated air,
Cleaner requires lead to swifter dreams—
An appendFile vanished in review streams!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive The PR description provides a clear summary and test plan, but omits critical template sections like Files Changed and formal Testing checkboxes. Complete the template by adding Files Changed section, checking the Testing checkboxes with actual test results, and confirming the cross-model review assessment.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: removing an unused import and the desired outcome of restoring the green Tests badge.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/v0.20.2-ci-typecheck-unused-import

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes an unused appendFile import from tests/demo/failure-gates.test.ts so CI's tsc --noEmit step stops failing with TS6133, restoring the green Tests badge.

Changes:

  • Drop appendFile from the node:fs/promises import list in the failure-gates test.

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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request removes the unused appendFile import from node:fs/promises in the tests/demo/failure-gates.test.ts file. The reviewer suggested further consolidating the imports by merging the realpath import into the main node:fs/promises block to improve code clarity and maintainability.


import { describe, test, expect, beforeEach, afterEach } from 'bun:test'
import { mkdtemp, mkdir, rm, writeFile, appendFile } from 'node:fs/promises'
import { mkdtemp, mkdir, rm, writeFile } from 'node:fs/promises'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The import of realpath from node:fs/promises on line 27 is redundant with this import statement. They should be consolidated into a single import from node:fs/promises to improve code clarity and maintainability. Please remember to remove line 27 after applying this change.

Suggested change
import { mkdtemp, mkdir, rm, writeFile } from 'node:fs/promises'
import { mkdtemp, mkdir, rm, writeFile, realpath } from 'node:fs/promises'

The CI `bun test` step was discovering `code-oz-gui/tests/unit/*.test.ts`,
which depend on `@google/genai` — a dependency of the GUI sub-package,
not the root. CI's `bun install --frozen-lockfile` only installs the
root deps, so the import failed with "Cannot find module '@google/genai'"
on both ubuntu-latest and macos-latest. Locally the GUI's
`node_modules` were present from earlier `bun install`s in that subdir,
which masked the failure.

Fix: scope the root `bun test` invocation (both in CI and in the
`test` / `test:watch` npm scripts) to `./tests`. The GUI's unit tests
remain runnable from inside `code-oz-gui/` via its own test runner.

This restores green Tests + Release badges in the README hero, which
is the visible signal that matters for first-time visitors. The static
test-count badge updates from 3395 → 3390 (the 5 GUI unit tests now
run only from inside the GUI sub-package).

Refs: companion to the unused-appendFile-import fix in the same PR
(the earlier typecheck failure was masking this test discovery bug).
@omerakben
omerakben merged commit a990a0e into main May 14, 2026
3 checks passed
@omerakben
omerakben deleted the fix/v0.20.2-ci-typecheck-unused-import branch May 14, 2026 17:14

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 04e954fece

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


- name: Test
run: bun test
run: bun test ./tests

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep GUI unit tests in CI by avoiding path-scoped bun test

This change narrows the CI command from bun test to bun test ./tests, which drops the code-oz-gui/tests/unit/* suite from the default test workflow with no replacement job in .github/workflows/ (I checked both test.yml and release.yml). Bun’s own CLI contract is bun test [<patterns>] (“run all matching test files”), so passing ./tests intentionally filters to that subtree; in this repo that reduces coverage from 3395 to 3390 passing tests and allows GUI regressions to merge undetected.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants