Add report to track the age of automated tests for the API#473
Merged
tracygardner merged 4 commits intoflipcomputing:mainfrom Mar 28, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdjusted Playwright launch to enable legacy headless mode; added a new executable script that builds an API-to-test coverage matrix using Git timestamps and writes a Markdown report; tests HTML now loads Mocha/Chai from local Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant FS as Filesystem
participant Script as test-coverage-matrix.mjs
participant Git as Git
participant Reporter as Reporter
rect rgba(200,230,250,0.5)
Dev->>Script: run generateTestCoverageMatrix()
end
Script->>FS: list api/*.js and tests/*.test.js
FS-->>Script: return file lists
Script->>Script: match APIs to tests (heuristics + special cases)
Script->>FS: read matched test files to count tests
FS-->>Script: return test counts
Script->>Git: git log -1 --format=%cI for files
Git-->>Script: return timestamps
Script->>Git: git log --after=... --oneline to count commits since
Git-->>Script: return commit counts
Script->>Reporter: assemble matrix, staleness, totals
Reporter->>FS: write reports/test-coverage-matrix.md
Reporter-->>Dev: print console table and warnings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 2
🧹 Nitpick comments (2)
scripts/test-coverage-matrix.mjs (1)
64-93: This becomes a third source of truth for suite mapping.
tests/tests.htmlandscripts/run-api-tests.mjsalready carry suite metadata. Rebuilding that relationship here with filename heuristics will drift from the runner over time, and thetestFile.includes('ui')branch is loose enough to create false positives. I'd extract a shared manifest and add explicitapiFilesmetadata to it instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-coverage-matrix.mjs` around lines 64 - 93, The findMatchingTestFiles function currently reimplements suite mapping with fragile filename heuristics (notably the loose testFile.includes('ui') branch); instead, create or import a single shared manifest (used by tests/tests.html and scripts/run-api-tests.mjs) that includes explicit apiFiles metadata per test suite, then change findMatchingTestFiles to consult that manifest to return exact matches for the given apiFileName (remove heuristic branches like pluralization, prefix tries, the sound/ui special cases), and update scripts/run-api-tests.mjs and tests/tests.html to read the same manifest so the mapping is single-source-of-truth.tests/tests.html (1)
80-83: Standardize the Babylon suite interface and drop this global shim.
tests/babylon.test.jsis the only suite still using a differentrunTests(flock, expect)signature. If it matches the other test modules, thiswindow.chaibootstrap and the Babylon-only branch inloadAllTestscan both go away.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tests.html` around lines 80 - 83, Update tests/babylon.test.js so its exported runTests function matches the signature used by the other test modules (make it accept the same parameters/order as the other suites), remove any reliance on the global window.chai shim (drop usage of window.chai inside babylon.test.js), and then delete the Babylon-specific branch in loadAllTests and the window.chai bootstrap script in tests/tests.html; this lets loadAllTests call runTests uniformly for all suites without special-casing Babylon.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/test-coverage-matrix.mjs`:
- Around line 407-409: The direct-invocation check compares import.meta.url to a
handcrafted file:// string which fails on Windows and paths with special chars;
change the condition to compare the already-computed __filename against
process.argv[1] (or path.resolve both sides) and keep the call to
generateTestCoverageMatrix() unchanged so the script runs when invoked directly;
look for the existing import.meta.url usage and the compute of __filename via
fileURLToPath(import.meta.url) and replace the if condition accordingly.
- Around line 27-33: Both getGitLastModified and getCommitsSince are calling
execSync with interpolated filePath which is command-injection-prone and are
truncating %ai to date only; replace those execSync calls with execFileSync and
pass git and args as an array (e.g. ['log','-1','--format=%cI','--', filePath])
to avoid shell interpolation, request the full ISO timestamp using %cI instead
of %ai so you preserve time information for staleness checks, and keep the same
encoding/stdio options when calling execFileSync.
---
Nitpick comments:
In `@scripts/test-coverage-matrix.mjs`:
- Around line 64-93: The findMatchingTestFiles function currently reimplements
suite mapping with fragile filename heuristics (notably the loose
testFile.includes('ui') branch); instead, create or import a single shared
manifest (used by tests/tests.html and scripts/run-api-tests.mjs) that includes
explicit apiFiles metadata per test suite, then change findMatchingTestFiles to
consult that manifest to return exact matches for the given apiFileName (remove
heuristic branches like pluralization, prefix tries, the sound/ui special
cases), and update scripts/run-api-tests.mjs and tests/tests.html to read the
same manifest so the mapping is single-source-of-truth.
In `@tests/tests.html`:
- Around line 80-83: Update tests/babylon.test.js so its exported runTests
function matches the signature used by the other test modules (make it accept
the same parameters/order as the other suites), remove any reliance on the
global window.chai shim (drop usage of window.chai inside babylon.test.js), and
then delete the Babylon-specific branch in loadAllTests and the window.chai
bootstrap script in tests/tests.html; this lets loadAllTests call runTests
uniformly for all suites without special-casing Babylon.
🪄 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: 894f6075-abf5-4443-97e6-449e84e455e7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
scripts/run-api-tests.mjsscripts/test-coverage-matrix.mjstests/tests.html
julianharty
changed the title
julianharty
added a commit
to commercetest/flock-xr
that referenced
this pull request
Mar 27, 2026
…trix Replace execSync with execFileSync and pass git arguments as an array to eliminate shell interpolation of file paths. A malicious filename containing shell metacharacters could otherwise execute arbitrary commands. Also switch from %ai to %cI format to preserve full ISO 8601 timestamps, making same-day edits distinguishable in staleness tracking. Fix the direct-invocation guard to compare __filename against path.resolve(process.argv[1]) rather than constructing a file:// URL by hand. The previous comparison failed silently on paths with spaces or special characters. Flagged by coderabbitai in PR flipcomputing#473. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/test-coverage-matrix.mjs (2)
118-129: Consider validating directories exist before reading.If
api/ortests/directories don't exist,fs.readdirSyncthrows anENOENTerror with a less helpful message. Adding a pre-check would improve the developer experience.🔧 Suggested improvement
const apiDir = path.join(projectRoot, 'api'); const testsDir = path.join(projectRoot, 'tests'); + if (!fs.existsSync(apiDir)) { + console.error(`❌ API directory not found: ${apiDir}`); + return; + } + if (!fs.existsSync(testsDir)) { + console.error(`❌ Tests directory not found: ${testsDir}`); + return; + } + // Get all API files const apiFiles = fs.readdirSync(apiDir)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-coverage-matrix.mjs` around lines 118 - 129, Check that apiDir and testsDir exist before calling fs.readdirSync: use a directory existence check (e.g., fs.existsSync or fs.statSync) on the apiDir and testsDir variables and only call fs.readdirSync when they are present; if a directory is missing, handle gracefully by setting apiFiles or testFiles to an empty array (or logging a clear message) instead of letting fs.readdirSync throw; update the code around apiFiles, testFiles, and the uses of apiDir/testsDir to rely on those safe empty arrays.
86-92: Consider tightening theuimatching pattern.The condition
testFile.includes('ui')on line 89 is quite broad and could match unintended files (e.g., a hypotheticalcontinuity.test.jsorsecurity.test.js). A more specific pattern liketestFile.startsWith('ui')would be safer and consistent with thesoundspecial case above.🔧 Suggested refinement
} else if (baseName === 'sound' && testFile.startsWith('sound')) { // Special case: sound has multiple test files matches.push(testFile); - } else if (baseName === 'ui' && testFile.includes('ui')) { + } else if (baseName === 'ui' && testFile.startsWith('ui')) { // Special case: ui has uitextbutton.test.js matches.push(testFile); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-coverage-matrix.mjs` around lines 86 - 92, Tighten the UI special-case match: in the branch handling baseName === 'ui' (the block that currently checks testFile.includes('ui') and then calls matches.push(testFile)), replace the broad includes check with a more specific startsWith('ui') check so only test files beginning with "ui" (consistent with the sound case) are matched and pushed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/test-coverage-matrix.mjs`:
- Line 331: The markdown line that builds the "Files with Tests" percentage can
produce NaN when stats.totalApiFiles is 0; update the expression used in the md
+= template so it guards for zero (use 0% when stats.totalApiFiles === 0,
otherwise compute Math.round(stats.filesWithTests / stats.totalApiFiles * 100)).
Change the calculation in the md += `- **Files with Tests:** ${...}` template to
mirror the same division-by-zero guard you applied to the console output,
referencing stats.filesWithTests and stats.totalApiFiles.
- Around line 199-204: The percentage calculations can divide by zero when
apiFiles.length === 0; update the logging code around variables apiFiles,
filesWithTests, filesWithNoTests, and filesWithStaleTests to guard against that
by computing a safe denominator (e.g., const totalApi = apiFiles.length || 1 or
conditional: if apiFiles.length === 0 then percent = 0) and use that safe
denominator when calculating percentages so the output never becomes NaN and
instead shows 0% (or a defined fallback) for the filesWithTests/filesWithNoTests
lines.
---
Nitpick comments:
In `@scripts/test-coverage-matrix.mjs`:
- Around line 118-129: Check that apiDir and testsDir exist before calling
fs.readdirSync: use a directory existence check (e.g., fs.existsSync or
fs.statSync) on the apiDir and testsDir variables and only call fs.readdirSync
when they are present; if a directory is missing, handle gracefully by setting
apiFiles or testFiles to an empty array (or logging a clear message) instead of
letting fs.readdirSync throw; update the code around apiFiles, testFiles, and
the uses of apiDir/testsDir to rely on those safe empty arrays.
- Around line 86-92: Tighten the UI special-case match: in the branch handling
baseName === 'ui' (the block that currently checks testFile.includes('ui') and
then calls matches.push(testFile)), replace the broad includes check with a more
specific startsWith('ui') check so only test files beginning with "ui"
(consistent with the sound case) are matched and pushed.
🪄 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: bfb00646-00b3-4912-b526-bf8abd86b955
📒 Files selected for processing (1)
scripts/test-coverage-matrix.mjs
Playwright 1.44+ switched to a new headless shell binary that drops WebGL support. Tests that use Babylon.js (e.g. babylon, objects, materials) were failing locally on macOS with "WebGL not supported" because flock.scene was never initialised. Switching to headless:false with --headless=old restores the original headless Chromium mode which retains WebGL/WebGL2 support. This is consistent with what CI has been getting implicitly via the older Playwright build (v1193) it uses. Also fix tests/tests.html to load mocha and chai from local node_modules instead of unpkg.com CDN, which was blocked by the Vite dev server's Content-Security-Policy. Mocha is loaded as a classic script (UMD, sets window.mocha) and chai is imported as an ES module and assigned to window.chai. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements the docs:matrix npm script that generates a matrix showing: - API files mapped to their corresponding test files - Test counts per file - Git-based staleness tracking (commits since tests were updated) - Visual status indicators for test freshness Outputs both console report and markdown file to reports/test-coverage-matrix.md Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…trix Replace execSync with execFileSync and pass git arguments as an array to eliminate shell interpolation of file paths. A malicious filename containing shell metacharacters could otherwise execute arbitrary commands. Also switch from %ai to %cI format to preserve full ISO 8601 timestamps, making same-day edits distinguishable in staleness tracking. Fix the direct-invocation guard to compare __filename against path.resolve(process.argv[1]) rather than constructing a file:// URL by hand. The previous comparison failed silently on paths with spaces or special characters. Flagged by coderabbitai in PR flipcomputing#473. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Lines 200, 201, and 331 divided by apiFiles.length / stats.totalApiFiles without checking for zero, producing NaN in the percentage output if the script runs against a directory with no API files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/test-coverage-matrix.mjs (2)
240-244: ISO timestamps will overflow the 12-character column width.Full ISO 8601 timestamps from
%cI(e.g.,2024-03-26T17:01:06+00:00) are ~25 characters, but the "Last Modified" and "Test Modified" columns are padded to 12 characters. This will cause misaligned console output.Consider extracting just the date portion for console display while keeping full timestamps in the markdown report, or widen the columns:
💡 Optional: truncate to date for console display
const rowStr = [ row.apiFile.padEnd(16), - (row.apiLastModified || '-').padEnd(12), + (row.apiLastModified ? row.apiLastModified.split('T')[0] : '-').padEnd(12), testFileStr.padEnd(38), row.testCount.toString().padStart(5), - (row.testLastModified || '-').padEnd(12), + (row.testLastModified ? row.testLastModified.split('T')[0] : '-').padEnd(12), stalenessStr.padStart(5), statusIndicator ].join(' │ ');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-coverage-matrix.mjs` around lines 240 - 244, The console columns overflow because full ISO timestamps (row.apiLastModified, row.testLastModified) are ~25 chars but are padded to 12; fix by deriving display-only date strings (e.g., apiLastModifiedDisplay = row.apiLastModified ? row.apiLastModified.split('T')[0] : '-' and testLastModifiedDisplay = row.testLastModified ? row.testLastModified.split('T')[0] : '-') and use those display variables in the padEnd calls (replace row.apiLastModified and row.testLastModified in the console output) while leaving the original full timestamps intact for the markdown report generation.
118-129: Consider handling missing directories gracefully.If
api/ortests/directories don't exist,fs.readdirSyncwill throw anENOENTerror with no context. While this is acceptable for a developer tool expected to run within the repo, a clearer error message would improve the user experience.💡 Optional improvement
const apiDir = path.join(projectRoot, 'api'); const testsDir = path.join(projectRoot, 'tests'); + if (!fs.existsSync(apiDir)) { + console.error(`Error: API directory not found at ${apiDir}`); + process.exit(1); + } + if (!fs.existsSync(testsDir)) { + console.error(`Error: Tests directory not found at ${testsDir}`); + process.exit(1); + } + // Get all API files const apiFiles = fs.readdirSync(apiDir)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-coverage-matrix.mjs` around lines 118 - 129, The current calls to fs.readdirSync for apiDir and testsDir can throw ENOENT with no context; update the logic around the apiFiles and testFiles retrieval (references: apiDir, testsDir, apiFiles, testFiles) to handle missing directories gracefully by either checking existence first with fs.existsSync or wrapping each fs.readdirSync in a try/catch and throwing/logging a clearer error that includes the missing directory path and suggested action (e.g., "directory not found: <path> — ensure api/ exists or run from repo root"); ensure behavior remains sortable and that empty arrays are returned or the process exits with the new informative message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/test-coverage-matrix.mjs`:
- Around line 310-314: The coveragePercent calculation can divide by zero when
apiFiles is empty; update the logic around filesWithTests and apiFiles.length so
that if apiFiles.length === 0 you set coveragePercent to 100 (treating empty API
surface as fully covered) otherwise compute coveragePercent =
Math.round(filesWithTests / apiFiles.length * 100); keep the existing warning
print using coveragePercent.
- Around line 1-20: Prettier formatting in scripts/test-coverage-matrix.mjs is
failing CI; run the formatter (e.g., npx prettier --write
scripts/test-coverage-matrix.mjs) or apply equivalent style fixes so imports,
comments, and the top-level constants (__filename, __dirname, projectRoot, and
the import lines including countTestsInFile) follow the project's Prettier
rules; after reformatting, stage the updated file and push so the Prettier check
passes.
---
Nitpick comments:
In `@scripts/test-coverage-matrix.mjs`:
- Around line 240-244: The console columns overflow because full ISO timestamps
(row.apiLastModified, row.testLastModified) are ~25 chars but are padded to 12;
fix by deriving display-only date strings (e.g., apiLastModifiedDisplay =
row.apiLastModified ? row.apiLastModified.split('T')[0] : '-' and
testLastModifiedDisplay = row.testLastModified ?
row.testLastModified.split('T')[0] : '-') and use those display variables in the
padEnd calls (replace row.apiLastModified and row.testLastModified in the
console output) while leaving the original full timestamps intact for the
markdown report generation.
- Around line 118-129: The current calls to fs.readdirSync for apiDir and
testsDir can throw ENOENT with no context; update the logic around the apiFiles
and testFiles retrieval (references: apiDir, testsDir, apiFiles, testFiles) to
handle missing directories gracefully by either checking existence first with
fs.existsSync or wrapping each fs.readdirSync in a try/catch and
throwing/logging a clearer error that includes the missing directory path and
suggested action (e.g., "directory not found: <path> — ensure api/ exists or run
from repo root"); ensure behavior remains sortable and that empty arrays are
returned or the process exits with the new informative message.
🪄 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: 730d982b-5478-4a7d-9e37-68ce374f2ba2
📒 Files selected for processing (1)
scripts/test-coverage-matrix.mjs
julianharty
force-pushed
the
how-are-the-automated-tests
branch
from
7205723 to
7f729ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/tests.html`:
- Around line 81-83: Replace the hard-coded module path import ("
/node_modules/chai/index.js") with the package import string "chai" in the
import statement so the ESM resolver/bundler (Vite) uses Chai's official exports
map; keep the existing export to window.chai (the import statement and the
window.chai = chai assignment) unchanged so tests still access the global chai
instance.
🪄 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: 80a36ccb-43d9-49aa-9023-577d8f489524
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
scripts/run-api-tests.mjsscripts/test-coverage-matrix.mjstests/tests.html
✅ Files skipped from review due to trivial changes (1)
- scripts/test-coverage-matrix.mjs
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.
This adds a new report to assess whether various API tests lag updates to the respective APIs. Currently this report is run using a CLI command
npm run docs:matrix. The output appears in console output and is also written to./reports/test-coverage-matrix.md.Please let me know whether this report is useful and, if so, what'd improve its utility. I'll update the test documentation once I know the report is worthwhile.
Summary by CodeRabbit
New Features
Chores