test: create ci test summary#36860
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36860 +/- ##
===========================================
- Coverage 67.01% 67.00% -0.01%
===========================================
Files 3416 3416
Lines 117685 117685
Branches 21518 21514 -4
===========================================
- Hits 78861 78852 -9
- Misses 36152 36163 +11
+ Partials 2672 2670 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces reporters for Playwright and Mocha to emit structured JSON test summaries, updates E2E and CI workflows to produce, artifact, and merge per-matrix summaries, adds a Node script to consolidate summaries into Markdown and a GitHub job summary, and ignores generated files in the Meteor app. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as GitHub Actions Matrix (UI/API shards)
participant PW as Playwright Runner (UI)
participant MC as Mocha Runner (API)
participant RptPW as github-summary.ts
participant RptMC as github-summary-mocha.js
participant Art as upload-artifact
rect rgb(240,248,255)
note right of Dev: Each matrix shard sets CI=true and GITHUB_SUMMARY_PATH
Dev->>PW: Run UI tests
PW->>RptPW: Emit per-test events
RptPW-->>PW: Write test-summary-<...>.json
Dev->>MC: Run API tests (tee api-test-output.log)
MC->>RptMC: Emit per-test events
RptMC-->>MC: Write test-summary-api-<...>.json
end
rect rgb(245,255,240)
PW-->>Art: Upload UI summary artifact (if present)
MC-->>Art: Upload API summary artifact (if present)
end
sequenceDiagram
autonumber
actor CI as CI Workflow
participant DA as download-artifact
participant MS as merge-test-summaries.js
participant GHA as GHA Job Summary
participant FS as merged-test-summary.md
participant UA as upload-artifact
CI->>DA: Download test-summary-* (merge-multiple)
DA-->>CI: Files in workspace
CI->>MS: node .github/scripts/merge-test-summaries.js
MS->>MS: Discover & parse JSON summaries
MS->>MS: Merge suites & metadata
MS->>GHA: Append linked Markdown summary
MS->>FS: Write plain padded Markdown
CI->>UA: Upload merged-test-summary.md
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/meteor/reporters/mocha-github-summary.js (2)
73-79: Simplify the lineNumber logic.The current implementation attempts to extract line numbers from
test.bodybut always returnsnull. Since Mocha doesn't provide exact line numbers (as mentioned in the comment), this code can be simplified.// Get file path from the test let filePath = test.file; -let lineNumber = null; - -// Try to extract line number if available -if (test.body && typeof test.body === 'string') { - // This is a rough approximation - Mocha doesn't provide exact line numbers - // We could enhance this later if needed - lineNumber = null; -} +// Mocha doesn't provide exact line numbers +let lineNumber = null;
100-108: Consider using a for loop for better performance.Using
reduceto calculate totals can be replaced with a more performant for loop, especially when dealing with large test suites.// Calculate totals -const totals = summaries.reduce( - (acc, summary) => ({ - passed: acc.passed + summary.passed, - failed: acc.failed + summary.failed, - skipped: acc.skipped + summary.skipped, - total: acc.total + summary.total, - }), - { passed: 0, failed: 0, skipped: 0, total: 0 }, -); +const totals = { passed: 0, failed: 0, skipped: 0, total: 0 }; +for (const summary of summaries) { + totals.passed += summary.passed; + totals.failed += summary.failed; + totals.skipped += summary.skipped; + totals.total += summary.total; +}apps/meteor/reporters/github-summary.ts (1)
133-141: Consider using a for loop for better performance.Similar to the Mocha reporter, using
reducefor calculating totals can be replaced with a more performant for loop.// Calculate totals -const totals = summaries.reduce( - (acc, summary) => ({ - passed: acc.passed + summary.passed, - failed: acc.failed + summary.failed, - skipped: acc.skipped + summary.skipped, - total: acc.total + summary.total, - }), - { passed: 0, failed: 0, skipped: 0, total: 0 }, -); +const totals = { passed: 0, failed: 0, skipped: 0, total: 0 }; +for (const summary of summaries) { + totals.passed += summary.passed; + totals.failed += summary.failed; + totals.skipped += summary.skipped; + totals.total += summary.total; +}.github/scripts/merge-test-summaries.js (2)
293-296: Simplify the file matching logic.The current condition is redundant - if a file starts with 'test-summary-', it already includes 'test-summary'.
// Match various test summary file patterns (JSON files only) -if ((entry.name.includes('test-summary') || entry.name.startsWith('test-summary-')) && - entry.name.endsWith('.json')) { +if (entry.name.includes('test-summary') && entry.name.endsWith('.json')) {
419-427: Consider using a for loop for better performance.Similar to the reporters, using
reducefor calculating totals can be replaced with a more performant for loop.// Log test results summary (don't exit with error code for failed tests - that's handled elsewhere) -const totals = Array.from(allSuites.values()).reduce( - (acc, suite) => ({ - passed: acc.passed + suite.passed, - failed: acc.failed + suite.failed, - skipped: acc.skipped + suite.skipped, - total: acc.total + suite.total, - }), - { passed: 0, failed: 0, skipped: 0, total: 0 } -); +const totals = { passed: 0, failed: 0, skipped: 0, total: 0 }; +for (const suite of allSuites.values()) { + totals.passed += suite.passed; + totals.failed += suite.failed; + totals.skipped += suite.skipped; + totals.total += suite.total; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/scripts/merge-test-summaries.js(1 hunks).github/workflows/ci-test-e2e.yml(3 hunks).github/workflows/ci.yml(1 hunks)apps/meteor/.gitignore(1 hunks)apps/meteor/.mocharc.api.js(1 hunks)apps/meteor/playwright.config.ts(1 hunks)apps/meteor/reporters/github-summary.ts(1 hunks)apps/meteor/reporters/mocha-github-summary.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/meteor/reporters/github-summary.ts (1)
.github/scripts/merge-test-summaries.js (4)
result(358-358)totals(113-121)totals(419-427)report(18-18)
apps/meteor/reporters/mocha-github-summary.js (1)
.github/scripts/merge-test-summaries.js (3)
totals(113-121)totals(419-427)report(18-18)
.github/scripts/merge-test-summaries.js (1)
apps/meteor/reporters/mocha-github-summary.js (2)
fs(1-1)path(2-2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (8)
apps/meteor/.gitignore (1)
91-93: LGTM!The ignore patterns for test summary files align well with the new CI test summary generation feature.
apps/meteor/.mocharc.api.js (1)
14-18: LGTM!The CI-aware reporter configuration correctly wires up the custom GitHub summary reporter when running in CI mode.
.github/workflows/ci-test-e2e.yml (3)
238-239: LGTM!The GITHUB_SUMMARY_PATH environment variable correctly uses a unique naming pattern that includes release, MongoDB version, and shard to avoid conflicts between parallel test runs.
299-313: LGTM!The artifact upload steps correctly handle both UI and API test summaries with appropriate conditions and ignore missing files gracefully.
283-284: Fix missing CI environment variable.Line 284 sets
CI: truebut this appears to be misaligned - it should be at the same indentation level as GITHUB_SUMMARY_PATH.GITHUB_SUMMARY_PATH: test-summary-${{ inputs.release }}-${{ matrix.mongodb-version }}-${{ matrix.shard }}.json -CI: true + CI: trueLikely an incorrect or invalid review comment.
apps/meteor/playwright.config.ts (1)
25-31: LGTM!The conditional GitHub Summary Reporter configuration correctly integrates with the CI environment and properly handles the outputPath via environment variable.
.github/workflows/ci.yml (2)
539-580: LGTM!The test-summary job is well-structured with proper dependencies, conditions, and verification steps. The artifact upload handles failures gracefully with
if-no-files-found: warn.
584-584: LGTM!Correctly updated the tests-done job to depend on the new test-summary job.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
.github/scripts/merge-test-summaries.js (1)
21-33: JSON structure validation looks good; resolves the earlier concern.
🧹 Nitpick comments (8)
apps/meteor/reporters/github-summary.ts (4)
46-61: Key by suite + file to avoid name collisions across files.Identical suite titles in different files will be merged. Use a composite key; keep name unchanged for output.
- // Initialize or get existing summary - let summary = this.testResults.get(suiteName); + // Initialize or get existing summary (keyed by suite + file to avoid collisions) + const suiteKey = `${suiteName}::${filePath || 'unknown'}`; + let summary = this.testResults.get(suiteKey); if (!summary) { summary = { name: suiteName, @@ }; - this.testResults.set(suiteName, summary); + this.testResults.set(suiteKey, summary); }
80-86: Suite status handling is misleading for all-skipped suites.With no failures and only skipped tests, status remains 'passed'. Either include 'skipped' in the union or derive status at render-time only. Given the merger ignores this field, simplest is to compute status lazily when formatting.
145-150: Default metadata.type to 'ui'.Ensures consistent labeling when TEST_TYPE is unset.
- type: process.env.TEST_TYPE, + type: process.env.TEST_TYPE || 'ui',
100-105: Broaden test-file title filter.Skip typical test-file suite titles beyond .spec.ts/js (e.g., .test.ts, .tsx).
- // Skip filenames (they typically end with .spec.ts or .spec.js) - if (!suite.title.endsWith('.spec.ts') && !suite.title.endsWith('.spec.js')) { + // Skip filenames that look like test files + if (!/\.(spec|test)\.[tj]sx?$/.test(suite.title)) {.github/scripts/merge-test-summaries.js (2)
95-100: Encode branch and path segments in generated links.Avoid broken links on branches/paths with spaces or special chars.
- const githubUrl = `https://github.com/${repoOwner}/${repoName}/blob/${branch}/${relativePath}`; + const encBranch = encodeURIComponent(branch); + const encPath = relativePath.split('/').map(encodeURIComponent).join('/'); + const githubUrl = `https://github.com/${repoOwner}/${repoName}/blob/${encBranch}/${encPath}`;
409-411: Avoid dumping full summary to logs when not in CI.Large logs hinder troubleshooting. Log a short note instead.
- console.log('ℹ️ No GitHub Actions summary file found - skipping links, content:' + githubSummary); + console.log('ℹ️ No GitHub Actions summary file found - skipping linkified summary.');apps/meteor/.mocharc.api.js (1)
15-18: Only emit reporterOptions when the custom reporter is active.Prevents Mocha from warning about unused reporter options when using the 'spec' reporter.
- reporter: process.env.GITHUB_SUMMARY_PATH ? './reporters/mocha-github-summary.js' : 'spec', - reporterOptions: { - outputPath: process.env.GITHUB_SUMMARY_PATH || 'test-summary-api.json', - }, + reporter: process.env.GITHUB_SUMMARY_PATH ? './reporters/mocha-github-summary.js' : 'spec', + ...(process.env.GITHUB_SUMMARY_PATH + ? { reporterOptions: { outputPath: process.env.GITHUB_SUMMARY_PATH } } + : {}),apps/meteor/reporters/mocha-github-summary.js (1)
63-86: Suite-name collisions across files.Same top-level describe across multiple files will merge. Consider keying by name + file path (like suggested for the Playwright reporter).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/scripts/merge-test-summaries.js(1 hunks)apps/meteor/.mocharc.api.js(1 hunks)apps/meteor/reporters/github-summary.ts(1 hunks)apps/meteor/reporters/mocha-github-summary.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/reporters/github-summary.ts (1)
.github/scripts/merge-test-summaries.js (4)
result(357-357)totals(113-121)totals(418-418)report(18-18)
apps/meteor/reporters/mocha-github-summary.js (1)
.github/scripts/merge-test-summaries.js (5)
fs(3-3)path(4-4)totals(113-121)totals(418-418)report(18-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/reporters/mocha-github-summary.js (1)
130-140: Optional: include a default for metadata.type to 'api' (already set) and shard to be numeric.No change required; just confirming alignment with the merge script.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/ci-test-e2e.yml (3)
101-106: Fix bad redirection; you're creating a file named "sudo".Use a pipe into
sudo tee -ainstead of> sudo. Current form is broken and won’t set kernel params.- echo "500 65535" > sudo tee -a /proc/sys/net/ipv4/ip_local_port_range + echo "500 65535" | sudo tee -a /proc/sys/net/ipv4/ip_local_port_range
234-240: Add shard/type/release envs so summaries have consistent metadata.Converter/reporter expect shard/type/release. Set them here for API.
IS_EE: ${{ inputs.release == 'ee' && 'true' || '' }} COVERAGE_DIR: '/tmp/coverage' COVERAGE_REPORTER: 'lcovonly' GITHUB_SUMMARY_PATH: test-summary-api-${{ inputs.release }}-${{ matrix.mongodb-version }}-${{ matrix.shard }}.json CI: true + MOCHA_SHARD: ${{ matrix.shard }} + TEST_TYPE: api + TEST_RELEASE: ${{ inputs.release }}
272-293: Also provide metadata for Playwright UI summaries.Populate shard/type/release so
github-summary.tsemits complete metadata.QASE_REPORT: ${{ github.ref == 'refs/heads/develop' && 'true' || '' }} GITHUB_SUMMARY_PATH: test-summary-${{ inputs.release }}-${{ matrix.mongodb-version }}-${{ matrix.shard }}.json CI: true PLAYWRIGHT_RETRIES: ${{ inputs.retries }} + PLAYWRIGHT_SHARD: ${{ matrix.shard }} + TEST_TYPE: ui + TEST_RELEASE: ${{ inputs.release }}
♻️ Duplicate comments (1)
apps/meteor/reporters/github-summary.ts (1)
62-68: Count only the final attempt using object identity (more robust than index math).
result.retry === test.results.length - 1can be brittle. Compare the actual object to the last entry.- // Only count the final attempt to avoid double-counting retries - const isLastAttempt = typeof result.retry === 'number' ? result.retry === test.results.length - 1 : true; - if (!isLastAttempt) { - return; - } + // Only count the final attempt to avoid double-counting retries + const lastResult = test.results[test.results.length - 1]; + if (result !== lastResult) return;
🧹 Nitpick comments (2)
apps/meteor/reporters/convert-mocha-json.js (1)
122-129: CLI defaults: verify Mocha truly writes pure JSON to stdout.If tests emit console output,
> mocha-results.jsoncan corrupt JSON. Prefer a reporter that writes to file ormocha-multi-reporterswith a JSON file reporter.Would you like a patch to switch
npm run testapito usemocha-multi-reporters+ json-file output to avoid stdout contamination?apps/meteor/reporters/github-summary.ts (1)
97-129: Broaden spec-file filters.Playwright suites can end with .test.ts, .spec.tsx, .test.js, etc. Expand the filename checks to avoid mislabeling file nodes as suites.
- // Skip filenames (they typically end with .spec.ts or .spec.js) - if (!suite.title.endsWith('.spec.ts') && !suite.title.endsWith('.spec.js')) { + // Skip filenames (common patterns: .spec|.test.(ts|tsx|js|jsx)) + if (!/\.(spec|test)\.(ts|tsx|js|jsx)$/.test(suite.title)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/scripts/merge-test-summaries.js(1 hunks).github/workflows/ci-test-e2e.yml(4 hunks)apps/meteor/.mocharc.api.js(1 hunks)apps/meteor/reporters/convert-mocha-json.js(1 hunks)apps/meteor/reporters/github-summary.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/scripts/merge-test-summaries.js
- apps/meteor/.mocharc.api.js
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/reporters/convert-mocha-json.js (1)
.github/scripts/merge-test-summaries.js (5)
fs(3-3)path(4-4)totals(113-121)totals(420-420)report(18-18)
apps/meteor/reporters/github-summary.ts (2)
.github/scripts/merge-test-summaries.js (4)
result(359-359)totals(113-121)totals(420-420)report(18-18)apps/meteor/reporters/convert-mocha-json.js (6)
suiteName(21-21)suite(25-25)filePath(22-22)summaries(59-59)totals(65-65)report(74-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
apps/meteor/reporters/convert-mocha-json.js (2)
46-48: Treat all Mocha pending markers as skipped.Some Mocha JSONs set
pending: truewithoutstate. Your check already covers this; good to keep both branches.
85-94: Ensure parent dir creation covers absolute paths.The current
mkdirSyncguard (outputDir !== '.') is fine and safe. No change needed..github/workflows/ci-test-e2e.yml (2)
306-321: Artifacts: paths and names look consistent.Names include release/mongo/shard and paths point to apps/meteor/. Good.
241-259: Keep redirection — CI runs Mocha with the JSON reporter.
apps/meteor/.mocharc.api.js sets reporter toprocess.env.GITHUB_SUMMARY_PATH ? 'json' : 'spec', and the CI step in .github/workflows/ci-test-e2e.yml sets GITHUB_SUMMARY_PATH, so Mocha will emit JSON to stdout; redirecting to mocha-results.json is acceptable.
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 (1)
.github/workflows/ci-test-e2e.yml (1)
241-259: Pipe exit code is masked by tee; enforce pipefail and capture npm status.Without pipefail, the pipeline returns tee’s status (often 0), making failures look like passes. Capture npm’s exit code reliably.
Apply this diff:
- for i in $(seq 1 2); do - npm run testapi 2>&1 | tee api-test-output.log && s=0 && break || s=$? + set -o pipefail + for i in $(seq 1 2); do + npm run testapi 2>&1 | tee api-test-output.log + s=${PIPESTATUS[0]} + [ "$s" -eq 0 ] && break
🧹 Nitpick comments (1)
.github/workflows/ci-test-e2e.yml (1)
291-293: Name clarity: avoid confusion with GitHub’s GITHUB_STEP_SUMMARY.Consider TEST_SUMMARY_PATH or PLAYWRIGHT_SUMMARY_PATH to distinguish from GitHub’s built-in summary file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci-test-e2e.yml(4 hunks)apps/meteor/reporters/parse-mocha-output.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/reporters/parse-mocha-output.js (1)
121-167: Good CLI ergonomics and failure fallback.Solid error handling, directory creation, and a clear CLI path; exporting the parser enables reuse.
.github/workflows/ci-test-e2e.yml (2)
262-268: Nice: parse-and-persist API test summary post-run.Conditional parse with a soft-fail keeps the job robust while still emitting artifacts.
307-322: Artifact upload gates look correct.Per-matrix naming, always() guard, and ignore-if-missing are appropriate.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
apps/meteor/reporters/github-summary-mocha.js (5)
12-18: Defensive handling for options.Guard against undefined options and accept top-level options.output as a fallback.
-class GitHubSummaryMochaReporter { - constructor(runner, options) { +class GitHubSummaryMochaReporter { + constructor(runner, options = {}) { this.runner = runner; this.testResults = new Map(); - this.outputPath = process.env.GITHUB_SUMMARY_PATH || - (options.reporterOptions && options.reporterOptions.output) || - 'test-summary-mocha.json'; + this.outputPath = process.env.GITHUB_SUMMARY_PATH + || (options.reporterOptions && options.reporterOptions.output) + || options.output + || 'test-summary-mocha.json';
67-73: Mark all‑skipped suites as "skipped" (not "passed").Current logic leaves status "passed" when passed=0, failed=0, skipped>0.
- if (summary.failed > 0) { - summary.status = 'failed'; - } else if (summary.passed > 0 && summary.skipped > 0) { - summary.status = 'mixed'; - } + if (summary.failed > 0) { + summary.status = 'failed'; + } else if (summary.passed === 0 && summary.skipped > 0) { + summary.status = 'skipped'; + } else if (summary.passed > 0 && summary.skipped > 0) { + summary.status = 'mixed'; + }
79-105: Drop unused variable and improve filePath fallback.topLevelSuite is unused. Also provide a fallback to suite.file if test.file is undefined.
- let topLevelSuite; while (suite) { if (suite.title && suite.title !== '') { suiteTitles.push(suite.title); - // Keep track of the outermost describe block - topLevelSuite = suite; } suite = suite.parent; } - // Get file path from the test - const filePath = test.file; + // Get file path from the test or containing suite + const filePath = test.file || test.parent?.file || test.parent?.parent?.file || null;
110-112: Stable sort by file then suite (optional).This yields more consistent diffs across runs.
- summaries.sort((a, b) => a.name.localeCompare(b.name)); + summaries.sort((a, b) => + (a.filePath || '').localeCompare(b.filePath || '') || a.name.localeCompare(b.name) + );
122-133: Add a schemaVersion for downstream tooling (optional).Helps future-proof the merge script.
metadata: { timestamp: new Date().toISOString(), shard: process.env.MOCHA_SHARD || process.env.PLAYWRIGHT_SHARD || '1', type: process.env.TEST_TYPE || 'api', release: process.env.IS_EE === 'true' ? 'ee' : 'ce', reporter: 'github-summary-mocha', + schemaVersion: 1, },apps/meteor/package.json (2)
40-41: Avoid duplicate script; wire CI env explicitly.testapi:ci duplicates testapi. Either drop it or set CI flags here to differentiate CI/local.
- "testapi:ci": "TS_NODE_COMPILER_OPTIONS='{\"module\": \"commonjs\"}' mocha --config ./.mocharc.api.js", + "testapi:ci": "CI=1 TS_NODE_COMPILER_OPTIONS='{\"module\": \"commonjs\"}' mocha --config ./.mocharc.api.js",
76-76: Unused dependency detected.@mochajs/json-file-reporter isn’t configured in .mocharc.*; remove it or wire it up.
If not needed, remove:
- "@mochajs/json-file-reporter": "^1.3.0",apps/meteor/.mocharc.api.js (1)
9-10: Prefer object form for reporterOptions.Clearer and type‑safe; avoids string parsing pitfalls.
- reporter: 'mocha-multi-reporters', - reporterOptions: 'configFile=.mocharc.multi.json', + reporter: 'mocha-multi-reporters', + reporterOptions: { configFile: '.mocharc.multi.json' },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
apps/meteor/.mocharc.api.js(1 hunks)apps/meteor/.mocharc.multi.json(1 hunks)apps/meteor/package.json(3 hunks)apps/meteor/reporters/github-summary-mocha.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/reporters/github-summary-mocha.js (1)
.github/scripts/merge-test-summaries.js (1)
report(18-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/package.json (1)
195-196: Compatibility OK — mocha-multi-reporters@^1.5.1 is compatible with mocha@^9.2.2mocha-multi-reporters@1.5.1 declares peerDependencies: { "mocha": ">=3.1.2" }, so mocha@^9.2.2 satisfies that requirement. (unpkg.com)
The local node -e check failed because node_modules weren't installed; ensure CI runs npm ci / yarn install (including devDependencies) before running tests.
| "reporterEnabled": "spec, ./reporters/github-summary-mocha.js", | ||
| "reportersGithubSummaryMochaJsReporterOptions": { | ||
| "output": "test-summary-mocha-api.json" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Wrong per‑reporter options key; reporter won’t receive the output path.
For mocha-multi-reporters, the key is derived from the reporter’s base name: github-summary-mocha -> githubSummaryMochaReporterOptions.
{
- "reporterEnabled": "spec, ./reporters/github-summary-mocha.js",
- "reportersGithubSummaryMochaJsReporterOptions": {
+ "reporterEnabled": "spec, ./reporters/github-summary-mocha.js",
+ "githubSummaryMochaReporterOptions": {
"output": "test-summary-mocha-api.json"
}
}Confirm the reporter picks it up by checking CI logs for:
“Mocha test summary written to test-summary-mocha-api.json”.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== repo root: $(pwd) ==="
echo
echo "=== apps/meteor dir listing ==="
ls -la apps/meteor || true
echo
echo "=== show apps/meteor/.mocharc.multi.json (first 200 lines) ==="
if [ -f apps/meteor/.mocharc.multi.json ]; then
sed -n '1,200p' apps/meteor/.mocharc.multi.json
else
echo "MISSING: apps/meteor/.mocharc.multi.json"
fi
echo
echo "=== search repository for relevant keys & filename ==="
rg -n --hidden --no-ignore -S 'reportersGithubSummaryMochaJsReporterOptions|githubSummaryMochaReporterOptions|github-summary-mocha|test-summary-mocha-api.json|reporterEnabled' || trueLength of output: 4312
Fix reporter options key in apps/meteor/.mocharc.multi.json
mocha-multi-reporters derives per-reporter option keys from the reporter base name — replace reportersGithubSummaryMochaJsReporterOptions with githubSummaryMochaReporterOptions so the reporter receives the output path.
Location: apps/meteor/.mocharc.multi.json
{
- "reporterEnabled": "spec, ./reporters/github-summary-mocha.js",
- "reportersGithubSummaryMochaJsReporterOptions": {
+ "reporterEnabled": "spec, ./reporters/github-summary-mocha.js",
+ "githubSummaryMochaReporterOptions": {
"output": "test-summary-mocha-api.json"
}
}Verify by checking CI logs for: “Mocha test summary written to test-summary-mocha-api.json”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "reporterEnabled": "spec, ./reporters/github-summary-mocha.js", | |
| "reportersGithubSummaryMochaJsReporterOptions": { | |
| "output": "test-summary-mocha-api.json" | |
| } | |
| { | |
| "reporterEnabled": "spec, ./reporters/github-summary-mocha.js", | |
| "githubSummaryMochaReporterOptions": { | |
| "output": "test-summary-mocha-api.json" | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/meteor/.mocharc.multi.json around lines 2 to 5, the per-reporter options
key is incorrect: change reportersGithubSummaryMochaJsReporterOptions to
githubSummaryMochaReporterOptions so mocha-multi-reporters will pass the
"output": "test-summary-mocha-api.json" option to the github-summary reporter;
update the key name only (preserve the output value) and commit, then verify CI
shows “Mocha test summary written to test-summary-mocha-api.json”.
| // Bind event handlers | ||
| this.runner.on('test end', this.onTestEnd.bind(this)); | ||
| this.runner.on('end', this.onEnd.bind(this)); | ||
| } |
There was a problem hiding this comment.
Skipped tests aren’t counted; listen to Mocha’s "pending" event.
Mocha does not emit "test end" for pending tests and test.state is never "pending" on that event. As‑is, skipped totals and suite status will be wrong.
Apply:
constructor(runner, options) {
this.runner = runner;
this.testResults = new Map();
- this.outputPath = process.env.GITHUB_SUMMARY_PATH ||
- (options.reporterOptions && options.reporterOptions.output) ||
- 'test-summary-mocha.json';
+ this.outputPath = process.env.GITHUB_SUMMARY_PATH
+ || (options.reporterOptions && options.reporterOptions.output)
+ || (options && options.output)
+ || 'test-summary-mocha.json';
// Bind event handlers
this.runner.on('test end', this.onTestEnd.bind(this));
+ this.runner.on('pending', this.onPending.bind(this));
this.runner.on('end', this.onEnd.bind(this));
}
+onPending(test) {
+ const { suiteName, filePath } = this.getSuiteNameAndLocation(test);
+ const key = `${suiteName}::${filePath || 'unknown'}`;
+ let summary = this.testResults.get(key);
+ if (!summary) {
+ summary = { name: suiteName, passed: 0, failed: 0, skipped: 0, total: 0, status: 'passed', filePath, lineNumber: null };
+ this.testResults.set(key, summary);
+ }
+ summary.total++;
+ summary.skipped++;
+ summary.status = summary.failed > 0 ? 'failed' : (summary.passed > 0 ? 'mixed' : 'skipped');
+}Also applies to: 45-66
| // Initialize or get existing summary | ||
| let summary = this.testResults.get(suiteName); | ||
| if (!summary) { | ||
| summary = { | ||
| name: suiteName, | ||
| passed: 0, | ||
| failed: 0, | ||
| skipped: 0, | ||
| total: 0, | ||
| status: 'passed', | ||
| filePath, | ||
| lineNumber: null, // Mocha doesn't provide line numbers easily | ||
| }; | ||
| this.testResults.set(suiteName, summary); | ||
| } |
There was a problem hiding this comment.
Avoid suite key collisions across files.
Using only the outermost describe as the Map key merges suites with the same name from different files.
- let summary = this.testResults.get(suiteName);
+ const key = `${suiteName}::${filePath || 'unknown'}`;
+ let summary = this.testResults.get(key);
if (!summary) {
summary = {
name: suiteName,
passed: 0,
failed: 0,
skipped: 0,
total: 0,
status: 'passed',
filePath,
lineNumber: null, // Mocha doesn't provide line numbers easily
};
- this.testResults.set(suiteName, summary);
+ this.testResults.set(key, summary);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Initialize or get existing summary | |
| let summary = this.testResults.get(suiteName); | |
| if (!summary) { | |
| summary = { | |
| name: suiteName, | |
| passed: 0, | |
| failed: 0, | |
| skipped: 0, | |
| total: 0, | |
| status: 'passed', | |
| filePath, | |
| lineNumber: null, // Mocha doesn't provide line numbers easily | |
| }; | |
| this.testResults.set(suiteName, summary); | |
| } | |
| // Initialize or get existing summary | |
| const key = `${suiteName}::${filePath || 'unknown'}`; | |
| let summary = this.testResults.get(key); | |
| if (!summary) { | |
| summary = { | |
| name: suiteName, | |
| passed: 0, | |
| failed: 0, | |
| skipped: 0, | |
| total: 0, | |
| status: 'passed', | |
| filePath, | |
| lineNumber: null, // Mocha doesn't provide line numbers easily | |
| }; | |
| this.testResults.set(key, summary); | |
| } |
🤖 Prompt for AI Agents
In apps/meteor/reporters/github-summary-mocha.js around lines 29 to 43, the Map
key uses only the outermost suite name which causes suites with identical names
in different files to collide; change the key to a composite that includes
filePath (e.g. `${filePath}::${suiteName}` or similar unique delimiter) and
update the get/set calls to use that composite key everywhere, ensuring you
still store suiteName and filePath separately in the summary object so
downstream code can access both.
…lows and Playwright config
1 participant
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Tests
Chores