Skip to content

feat: add unit tests for trusted flue functions#32186

Open
mvvmm wants to merge 3 commits into
productionfrom
flue/unit-tests
Open

feat: add unit tests for trusted flue functions#32186
mvvmm wants to merge 3 commits into
productionfrom
flue/unit-tests

Conversation

@mvvmm

@mvvmm mvvmm commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds 127 Vitest unit tests covering the deterministic, pure TypeScript functions in .flue/ — the layer that doesn't require AI, GitHub API calls, R2, or Cloudflare Workers bindings to verify.

What's tested:

File Tests What it covers
lib/code-review-inproc.test.ts 29 parseAddedLines (diff parsing, hunk math, file headers, edge cases), selectCodeReviewFiles (exclusions, sorting, capping), mergeCodeReviewResults (dedup by ID, summary counts)
lib/code-review-render.test.ts 41 renderPendingComment, renderFailureComment, renderComment (all severity combinations, fix-in-agent block, acknowledged findings, degraded sections), renderReviewLimitComment, renderRebaseStatusUpdate (all status values, existing body injection, timestamp refresh)
lib/code-review-state.test.ts 16 extractReviewedHeadSha, extractReviewedAt, partitionComments (bot detection, last-wins, timestamp partitioning, bot-type exclusion)
lib/code-review-results.test.ts 11 assignCodeReviewFindingIds and assignFindingIds (CR-/SG- prefix, stable IDs, line-number independence, evidence trimming)
lib/inproc-utils.test.ts 8 withConcurrency (ordering, concurrency limit, NaN/zero clamping, serial execution)
lib/github-webhook.test.ts 22 getIssueOrPullRequestNumber/Url/Label/Title, truncateLogValue

Also adds vitest as a devDependency, a vitest.config.ts (with assetsInclude: ['**/*.md'] to handle SKILL.md imports), and wires pnpm run test into the Pre Build job in flue-ci.yml.

@github-actions

Copy link
Copy Markdown
Contributor

This pull request requires reviews from CODEOWNERS as it changes files that match the following patterns:

Pattern Owners
*.ts @cloudflare/content-engineering, @kodster28
package.json @cloudflare/content-engineering
* @cloudflare/product-owners
/.github/ @cloudflare/content-engineering, @kodster28, @mvvmm, @colbywhite, @ahaywood, @MohamedH1998

@mvvmm
mvvmm marked this pull request as ready for review July 20, 2026 21:49
@cloudflare-docs-bot

cloudflare-docs-bot Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review

⚠️ 1 warning, 💡 1 suggestion found in full PR diff.

👉 Fix in your agent 👈
Fix the following review findings in PR #32186 (https://github.com/cloudflare/cloudflare-docs/pull/32186).

Before making changes, review each finding and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g. the fix requires disproportionate effort for minimal benefit,
  or the finding is factually incorrect), explain why
- If you need clarification before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

After triaging, follow this order:
1. Post a comment on this PR for any findings you are skipping, with the finding ID and your reasoning.
2. Then commit the fixes for the legitimate findings.

The comment must come before the commit — the bot reads PR comments when a new
push triggers a review, so skip comments posted after the push will be missed.

---

## Code Review

### Warnings (1)

#### CR-49f39dc6347c · Weak test assertion
- **File:** `.flue/lib/inproc-utils.test.ts` line 45
- **Issue:** `expect(maxActive).toBeLessThanOrEqual(3)` would pass if `withConcurrency` accidentally ran tasks serially (active=1), so it does not validate that a limit of 3 actually produces 3 concurrent tasks.
- **Fix:** Change the assertion to `expect(maxActive).toBe(3)`, which is the expected value when 10 tasks are run with a concurrency limit of 3.

### Suggestions (1)

#### CR-44b60e3cc039 · Duplicate patch version of vitest
- **File:** `.flue/package.json` line 27
- **Issue:** The repository root already pins vitest to 4.1.9 in devDependencies; .flue now pins 4.1.10, which will install a second vitest patch in the lockfile.
- **Fix:** Use the same vitest version as the root workspace (4.1.9) unless 4.1.10 is strictly required, to avoid duplicate installs and inconsistent runner behavior.

Code Review

This code review is in beta and may not always be helpful — use your judgment.

Warnings (1)
File Issue
.flue/lib/inproc-utils.test.ts line 45 Weak test assertionexpect(maxActive).toBeLessThanOrEqual(3) would pass if withConcurrency accidentally ran tasks serially (active=1), so it does not validate that a limit of 3 actually produces 3 concurrent tasks. Fix: Change the assertion to expect(maxActive).toBe(3), which is the expected value when 10 tasks are run with a concurrency limit of 3.
Suggestions (1)
File Issue
.flue/package.json line 27 Duplicate patch version of vitest — The repository root already pins vitest to 4.1.9 in devDependencies; .flue now pins 4.1.10, which will install a second vitest patch in the lockfile. Fix: Use the same vitest version as the root workspace (4.1.9) unless 4.1.10 is strictly required, to avoid duplicate installs and inconsistent runner behavior.

Conventions

No convention issues found.

Style Guide Review

No style-guide issues found.


Acknowledged by author (1)
Review File Issue Note
Code .flue/vitest.config.ts line 6 Static asset treatment does not handle custom import assertions The author says skill imports are only reached through mocked modules in tests, so the custom assertion handling is not exercised and does not need a loader fix.
Commands

Only codeowners can run commands. Post a comment with the command to trigger it.

Command Description
/review Runs a review now. Incremental if a prior review exists, full if not.
/full-review Re-reviews the entire PR diff from scratch, ignoring incremental history. Useful after a rebase, when you want a fresh review, or if the bot gets out of sync and reports issues that no longer exist.
/ignore-review-limit Permanently lifts the 2-review automatic limit for this PR. Future pushes will trigger reviews as normal.
/disable-auto-review Stops automatic reviews from triggering on future pushes to this PR. Codeowners can still run /review or /full-review manually.
/rebase Rebases the PR branch against production. On conflict, attempts to resolve automatically using AI. Stops with an explanation if confidence is not high enough.

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

@mvvmm

mvvmm commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

CR-d079110f46a3 / CR-086f81e0ea3f — both findings assume the root vitest.config.ts governs .flue/ tests. It does not. .flue/ is an independent pnpm workspace with its own vitest.config.ts that uses include: ["**/*.test.ts"]. CI runs pnpm run test from the .flue/ working directory (working-directory: .flue in flue-ci.yml), so Vitest resolves .flue/vitest.config.ts — not the root config. All test files are discovered correctly.

CR-a09afc0a3e69 — the concern is valid in isolation but doesn't apply here. Every module that calls createAgent (and therefore uses the skill import) is mocked at the top of code-review-inproc.test.ts via vi.mock. No test path reaches createAgent or consumes the skill value. Tests cover only pure data-transformation functions.

CR-d0259a0a9f76 — the build job has no test step. Tests run once, in pre-build only (lines 46–47 of flue-ci.yml). The build job runs pnpm run build only.

CR-bdd20c2650d1 — no duplicate. Lines 8 and 9 of ci.yml's paths-ignore are .flue/** and .github/workflows/flue-ci.yml — two distinct entries.

@mvvmm

mvvmm commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

/review

@mvvmm

mvvmm commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

/rebase

@mvvmm

mvvmm commented Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

/full-review

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants