test: adding integration tests setup #4353
Conversation
sagarkori143
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg and
sambhavgupta0705
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRemoves integration tests from Jest config, restructures npm test scripts, modifies runners: build-pages-runner now unconditionally self-executes and is no longer exported; build-post-list-runner accepts CLI args for output path/basePath. Adds a new integration test that runs both runners end-to-end and validates generated posts JSON. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CI as Developer/CI
participant BPR as build-pages-runner.ts
participant FS as File System
CI->>BPR: run module (node/tsx)
note right of BPR #D7F3E3: Top-level async IIFE now\ninvokes runBuildPages unconditionally
activate BPR
BPR->>FS: runBuildPages(...) → write built pages to basePath
BPR-->>CI: exit(0) on success / exit(1) on error
deactivate BPR
sequenceDiagram
autonumber
actor Jest as Integration Test
participant Tmp as Temp Dir
participant BPR as build-pages-runner.ts
participant BPLR as build-post-list-runner.ts
participant FS as File System
Jest->>Tmp: create temp basePath
Jest->>BPR: execute runner
BPR->>FS: generate pages into basePath
BPR-->>Jest: complete
Jest->>BPLR: execute with --outputFile posts.json --basePath <temp>
BPLR->>FS: read built pages
BPLR->>FS: write posts.json
BPLR-->>Jest: complete
Jest->>FS: read posts.json
Jest-->>Jest: assert structure (docs, blog, about, docsTree) and slugs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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 |
sagarkori143
changed the title
…ges directory for building posts
sagarkori143
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## add-integration-tests #4353 +/- ##
==========================================================
- Coverage 100.00% 27.05% -72.95%
==========================================================
Files 21 21
Lines 828 828
Branches 165 165
==========================================================
- Hits 828 224 -604
- Misses 0 577 +577
- Partials 0 27 +27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
akshatnema
added
the
gsoc
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
| outputPath = path.join(tempDir, outputFileName); | ||
|
|
||
| // Run the build-pages-runner before build-post-list-runner | ||
| execSync('npx tsx npm/runners/build-pages-runner.ts', { stdio: 'inherit' }); |
There was a problem hiding this comment.
Can't we directly run npm scripts?
There was a problem hiding this comment.
We can't use an npm script here because the test creates a unique temporary directory for each run and expects the runner to write the output file inside that directory.
There was a problem hiding this comment.
That's why for every command I am passing the base path and file name for using it as a temp location for the outputs.
✅ Actions performedFull review triggered. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
npm/runners/build-post-list-runner.ts (2)
31-38: Honor basePath when deriving default directoriesDefaults for postDirectories are hardcoded relative to currentDirPath, so providing options.basePath won’t affect discovery. Compute basePath first, then derive directories from it. This is important when the CLI passes a basePath.
- // Use provided options or fall back to default production paths - const postDirectories = options.postDirectories || [ - [resolve(currentDirPath, '../../pages/blog'), '/blog'], - [resolve(currentDirPath, '../../pages/docs'), '/docs'], - [resolve(currentDirPath, '../../pages/about'), '/about'] - ]; - const basePath = options.basePath || resolve(currentDirPath, '../../pages'); + // Use provided options or fall back to default production paths + const basePath = options.basePath || resolve(currentDirPath, '../../pages'); + const postDirectories = options.postDirectories || [ + [resolve(basePath, 'blog'), '/blog'], + [resolve(basePath, 'docs'), '/docs'], + [resolve(basePath, 'about'), '/about'] + ]; const outputPath = options.outputPath || resolve(currentDirPath, '../../config', 'posts.json');
53-77: CLI bug: --basePath is ignored and the runner always uses defaults; also add “main” guard and minimal arg validation
- The CLI computes outputPath from --basePath but never passes basePath into runBuildPostList, so the script still uses the default pages base. This is surprising and breaks the expected contract of the option.
- The runner self-executes on import. If any module imports this file for types or helpers, you’ll trigger a build as a side effect.
- If a flag is present without a value (e.g.,
--outputFileat the end), you’ll readundefined.Fix: guard execution to “run when main”, minimally validate args, and pass basePath through.
-// Self-executing async function to handle top-level await -(async () => { - try { - // Extract the file name and basePath from the CLI command - const outputFileArgIndex = process.argv.indexOf('--outputFile'); - const outputFileName = outputFileArgIndex === -1 ? 'posts.json' : process.argv[outputFileArgIndex + 1]; - - const basePathArgIndex = process.argv.indexOf('--basePath'); - const basePath = basePathArgIndex === -1 ? undefined : process.argv[basePathArgIndex + 1]; - - // Build outputPath using resolve(basePath, outputFileName) - let outputPath; - - if (basePath) { - outputPath = resolve(basePath, outputFileName); - } else { - outputPath = resolve(currentDirPath, '../../config', outputFileName); - } - - await runBuildPostList({ outputPath }); - } catch (error) { - // Ensure we exit with error code - process.exit(1); - } -})(); +// Run only when invoked directly (not when imported) +if (fileURLToPath(import.meta.url) === process.argv[1]) { + (async () => { + try { + const getFlagValue = (flag: string, def?: string) => { + const idx = process.argv.indexOf(flag); + if (idx === -1) return def; + const val = process.argv[idx + 1]; + if (!val || val.startsWith('--')) return def; + return val; + }; + + const outputFileName = getFlagValue('--outputFile', 'posts.json')!; + const basePath = getFlagValue('--basePath', undefined); + + const outputPath = basePath + ? resolve(basePath, outputFileName) + : resolve(currentDirPath, '../../config', outputFileName); + + await runBuildPostList({ outputPath, basePath }); + } catch (error) { + // Ensure we exit with error code + process.exit(1); + } + })(); +}Note: fileURLToPath is already imported in this file. If you prefer a stronger check, compare against
new URL(process.argv[1], 'file:').href === import.meta.url. I can provide that variant too.
🧹 Nitpick comments (7)
jest.config.js (1)
16-20: Comment and behavior mismatch: integration tests are not excluded by defaultThe comment says “Default: all tests except integration”, but the config still matches all tests under tests/, including tests/integration. Also, Jest’s testMatch does not support negation globs like '!/netlify//.test.'—use testPathIgnorePatterns instead.
Apply this diff to make the behavior match the comment and avoid relying on unsupported negation:
- // Default: all tests except integration - testMatch: [ - '**/tests/**/*.test.*', - '!**/netlify/**/*.test.*' - ], + // Default: all unit tests; integration and netlify tests are excluded here and run via dedicated scripts + testMatch: ['**/tests/**/*.test.*'], + testPathIgnorePatterns: ['<rootDir>/tests/integration', '<rootDir>/netlify'],Follow-up:
- Ensure package.json:test:integration still explicitly targets tests/integration so those tests run when intended.
- This aligns with the learning that coverage should target scripts and not other suites by default.
npm/runners/build-post-list-runner.ts (2)
11-15: Tighten types for postDirectoriespostDirectories is a list of [fsPath, routePrefix] pairs. Use a tuple for each entry to capture intent and improve type safety.
-interface BuildPostListOptions { - postDirectories?: string[][]; +interface BuildPostListOptions { + postDirectories?: [string, string][]; basePath?: string; outputPath?: string; }
41-49: Improve error context with resolved outputPathWhen options.outputPath is undefined, the error detail shows “undefined”. Prefer the actual resolved outputPath for better diagnostics.
- const customError = CustomError.fromError(error, { + const customError = CustomError.fromError(error, { category: 'script', operation: 'runBuildPostList', - detail: `Build post list failed with output path: ${options.outputPath}` + detail: `Build post list failed with output path: ${options.outputPath ?? outputPath}` });tests/integration/build-post-list.int.test.ts (3)
20-22: Surface runner output for easier debuggingForward stdio for the second execSync as well; otherwise failures don’t show logs.
- execSync(`npx tsx npm/runners/build-post-list-runner.ts --outputFile ${outputFileName} --basePath "${tempDir}"`); + execSync(`npx tsx npm/runners/build-post-list-runner.ts --outputFile ${outputFileName} --basePath "${tempDir}"`, { stdio: 'inherit' });Optional: consider execFileSync to avoid shell parsing and improve portability. Example:
// execFileSync('npx', ['tsx', 'npm/runners/build-post-list-runner.ts', '--outputFile', outputFileName, '--basePath', tempDir], { stdio: 'inherit' });
62-71: Make slug assertions resilient to trailing slashDepending on upstream changes, slugs may include a trailing slash. Relax the expectation to keep the test stable but still strict.
- slug: '/docs' + slug: expect.stringMatching(/^\/docs\/?$/)
72-81: Same for About slug: allow optional trailing slashMirror the change for the about section.
- slug: '/about' + slug: expect.stringMatching(/^\/about\/?$/)package.json (1)
11-11: Ensure coverage excludes integration teststest:coverage currently runs Jest with default config, which (per current jest.config.js) may include integration tests. To align with the “coverage for scripts only” learning and the new unit/integration split, delegate coverage to the unit suite.
- "test:coverage": "jest --coverage", + "test:coverage": "npm run test:unit -- --coverage",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
jest.config.js(1 hunks)npm/runners/build-pages-runner.ts(1 hunks)npm/runners/build-post-list-runner.ts(1 hunks)package.json(1 hunks)tests/integration/build-post-list.int.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: akshatnema
PR: asyncapi/website#3101
File: tests/build-rss.test.js:25-27
Timestamp: 2024-11-01T09:55:20.531Z
Learning: In `tests/build-rss.test.js`, replacing `jest.resetModules()` with `jest.resetAllMocks()` in the `afterEach()` block causes errors. It is necessary to use `jest.resetModules()` to reset the module registry between tests in this file.
📚 Learning: 2024-11-10T18:16:22.273Z
Learnt from: vishvamsinh28
PR: asyncapi/website#3284
File: scripts/build-post-list.js:49-53
Timestamp: 2024-11-10T18:16:22.273Z
Learning: In `scripts/build-post-list.js`, the `walkDirectories` function depends on sequential execution because it mutates `resultObj`. Parallelizing the loop with `Promise.all` causes tests to fail.
Applied to files:
tests/integration/build-post-list.int.test.tsnpm/runners/build-post-list-runner.ts
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is enforced by the project's configuration which uses `moduleResolution: "bundler"` in tsconfig.json and TypeScript-aware ESLint plugins. The .ts extensions are required even though the files are imported using CommonJS require statements.
Applied to files:
jest.config.js
📚 Learning: 2024-12-30T11:00:42.064Z
Learnt from: JeelRajodiya
PR: asyncapi/website#3423
File: jest.config.cjs:6-7
Timestamp: 2024-12-30T11:00:42.064Z
Learning: The user only wants coverage for scripts, not for .tsx files, because the existing tests are focused on scripts.
Applied to files:
jest.config.js
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is a project-specific requirement enforced by the linting rules and build setup, even though it differs from typical Node.js behavior.
Applied to files:
jest.config.js
📚 Learning: 2024-11-01T09:35:23.912Z
Learnt from: akshatnema
PR: asyncapi/website#3101
File: scripts/build-rss.js:4-6
Timestamp: 2024-11-01T09:35:23.912Z
Learning: Converting `getAllPosts()` to asynchronous in `scripts/build-rss.js` causes the existing build system to fail.
Applied to files:
npm/runners/build-post-list-runner.ts
📚 Learning: 2025-06-19T13:51:27.459Z
Learnt from: sagarkori143
PR: asyncapi/website#4192
File: npm/runners/build-pages-runner.ts:7-14
Timestamp: 2025-06-19T13:51:27.459Z
Learning: In the AsyncAPI website build system, individual runner functions use try-catch blocks that rethrow errors to maintain consistent error boundaries, while the top-level orchestrator in npm/index.ts provides meaningful error logging with task-specific context using logger.error(`Error building ${taskName}:`, error). This separation of concerns allows for clean error handling at different architectural layers.
Applied to files:
npm/runners/build-post-list-runner.ts
🔇 Additional comments (4)
npm/runners/build-post-list-runner.ts (1)
28-51: Good separation of concerns and error boundaryrunBuildPostList wraps script invocation, adds contextual CustomError, logs, and rethrows—consistent with the project’s runner pattern noted in past learnings. Once the CLI issues are addressed, the structure looks solid.
tests/integration/build-post-list.int.test.ts (1)
102-112: Nice end-to-end checks on slug prefixesVerifying section-specific path prefixes is a good, low-cost invariants check that will catch routing regressions.
npm/runners/build-pages-runner.ts (1)
25-45: Runner structure and error propagation look goodConsistent with the project’s error-handling practice: log contextual CustomError and rethrow for the top-level to exit with a non-zero code.
package.json (1)
10-15: Scripts split looks goodSequential unit then integration runs make the intent explicit, and the dedicated runner test entry is helpful for local debugging.
npm/runners/build-pages-runner.ts
Outdated
| (async () => { | ||
| try { | ||
| await runBuildPages(); | ||
| } catch (error) { | ||
| // Ensure we exit with error code | ||
| process.exit(1); | ||
| } | ||
| })(); |
There was a problem hiding this comment.
Avoid auto-executing on import; add a “run when main” guard
Self-invoking at module load introduces hard-to-debug side effects if this file is ever imported. Prefer a main guard so imports remain pure and the CLI still works.
-(async () => {
- try {
- await runBuildPages();
- } catch (error) {
- // Ensure we exit with error code
- process.exit(1);
- }
-})();
+if (fileURLToPath(import.meta.url) === process.argv[1]) {
+ (async () => {
+ try {
+ await runBuildPages();
+ } catch (error) {
+ // Ensure we exit with error code
+ process.exit(1);
+ }
+ })();
+}Add this import at the top of the file:
import { fileURLToPath } from 'url';Optional: If other code needs to call this programmatically, consider exporting the function:
export async function runBuildPages(…)🤖 Prompt for AI Agents
In npm/runners/build-pages-runner.ts around lines 47 to 54, the module currently
self-invokes an async IIFE at import time which causes side effects; replace the
IIFE with a "run when main" guard so the module only auto-runs when executed
directly: export the runBuildPages function (if not already exported) and add a
top-file import of fileURLToPath from 'url'; then wrap the invocation in a
conditional that checks the module is the entry script (using
fileURLToPath(import.meta.url) compared to process.argv[1] or an equivalent
main-check) and call runBuildPages() inside that block so importing the file is
side-effect free.
| import { execSync } from 'child_process'; | ||
| import fs from 'fs'; | ||
| import os from 'os'; | ||
| import path from 'path'; | ||
|
|
||
| describe('Integration: build-post-list-runner CLI', () => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Increase Jest timeout for integration flow
The two tsx invocations can exceed the default 5s Jest timeout, especially on CI. Bump the timeout to avoid flakiness.
import { execSync } from 'child_process';
import fs from 'fs';
import os from 'os';
import path from 'path';
+// Integration builds can take longer on CI
+jest.setTimeout(60_000);
+
describe('Integration: build-post-list-runner CLI', () => {📝 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.
| import { execSync } from 'child_process'; | |
| import fs from 'fs'; | |
| import os from 'os'; | |
| import path from 'path'; | |
| describe('Integration: build-post-list-runner CLI', () => { | |
| import { execSync } from 'child_process'; | |
| import fs from 'fs'; | |
| import os from 'os'; | |
| import path from 'path'; | |
| // Integration builds can take longer on CI | |
| jest.setTimeout(60_000); | |
| describe('Integration: build-post-list-runner CLI', () => { |
🤖 Prompt for AI Agents
In tests/integration/build-post-list.int.test.ts around lines 1 to 6, the
integration test can exceed Jest's default 5s timeout when invoking tsx twice;
add a longer timeout for the suite (for example call jest.setTimeout(30000) at
the top of the file or inside the describe block) so the CLI flow has enough
time to complete on CI and avoid flaky failures.
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 (1)
npm/runners/build-post-list-runner.ts (1)
28-39: Default postDirectories should derive from the resolved basePathWhen a custom
basePathis supplied, defaults forpostDirectoriesshould be computed relative to thatbasePath, not tocurrentDirPath. Otherwise, a caller cannot relocate the scan root without also passing custompostDirectories.Proposed change outside the diff range (adjust inside
runBuildPostList):const basePath = options.basePath || resolve(currentDirPath, '../../pages'); const postDirectories = options.postDirectories || [ [resolve(basePath, 'blog'), '/blog'], [resolve(basePath, 'docs'), '/docs'], [resolve(basePath, 'about'), '/about'] ];
♻️ Duplicate comments (2)
npm/runners/build-pages-runner.ts (1)
47-54: Avoid auto-executing on import; guard with “run when main”Self-invoking at module load causes side effects when this module is imported (including by tests). Add a main guard so imports remain pure, and the CLI still works.
Apply this diff:
-(async () => { - try { - await runBuildPages(); - } catch (error) { - // Ensure we exit with error code - process.exit(1); - } -})(); +if (fileURLToPath(import.meta.url) === process.argv[1]) { + (async () => { + try { + await runBuildPages(); + } catch (error) { + // Ensure we exit with error code + logger.error('Build pages CLI failed', error); + process.exit(1); + } + })(); +}Add this import near the top:
import { fileURLToPath } from 'url';tests/integration/build-post-list.int.test.ts (1)
1-6: Increase Jest timeout for the integration flowTwo tsx invocations commonly exceed the default 5s timeout on CI. Set a higher timeout for stability.
import { execSync } from 'child_process'; import fs from 'fs'; import os from 'os'; import path from 'path'; +// Integration builds can take longer on CI +jest.setTimeout(60_000);
🧹 Nitpick comments (6)
npm/runners/build-post-list-runner.ts (1)
73-77: Exit-on-error swallows pre-run parsing failures; log before exitErrors thrown before
runBuildPostList(e.g., bad CLI args) are not logged. Log the error to aid debugging.} catch (error) { - // Ensure we exit with error code + // Ensure we exit with error code and visible diagnostics + logger.error('Build post list CLI failed', error); process.exit(1); }npm/runners/build-pages-runner.ts (1)
25-45: runBuildPages Is Only Used Internally, Exporting Is Non-Breaking but RecommendedVerification shows that
runBuildPagesis only invoked within its own module (lines 49–50) and has no external imports or references across the codebase—so exporting it will not break existing usages.• No occurrences of
runBuildPages(outsidenpm/runners/build-pages-runner.tswere found.
• Exporting the function and wiring up CLI flags remains an optional refactor to improve testability.Suggested changes:
--- a/npm/runners/build-pages-runner.ts +++ b/npm/runners/build-pages-runner.ts @@ -export async function runBuildPages(options: BuildPagesOptions = {}): Promise<void> { +export async function runBuildPages(options: BuildPagesOptions = {}): Promise<void> { const config = { ...DEFAULT_OPTIONS, ...options }; try { ensureDirectoryExists(config.targetDir); @@ -} + +} + +// --- CLI entrypoint --- +if (require.main === module) { + // Lightweight CLI parsing (you can swap for yargs/commander) + const argv = require('minimist')(process.argv.slice(2), { + string: ['sourceDir', 'targetDir'], + default: { sourceDir: DEFAULT_OPTIONS.sourceDir, targetDir: DEFAULT_OPTIONS.targetDir }, + }); + + runBuildPages({ sourceDir: argv.sourceDir, targetDir: argv.targetDir }) + .catch(err => { + process.exitCode = 1; + }); +}By exporting
runBuildPagesand exposing--sourceDir/--targetDir, integration tests can import the function directly and pass temporary directories without touching real folders.tests/integration/build-post-list.int.test.ts (2)
17-22: Use npm scripts or parameterizable runners to avoid hardcoding tsx callsDirect
npx tsxcalls work, but:
- They bypass any project-level wrappers (env, logging).
- Quoting is brittle across shells.
- With the current runner, pages are always written to the real
pages/dir, harming isolation.Either:
- Add minimal CLI flags to build-pages-runner (e.g.,
--targetDir "${tempDir}") and use those here, or- Expose
runBuildPagesand invoke it programmatically with a temp target.Also, mirror stdio on the second exec for consistent diagnostics.
- // Run the runner as a CLI command with the test output file name and basePath - execSync(`npx tsx npm/runners/build-post-list-runner.ts --outputFile ${outputFileName} --basePath "${tempDir}"`); + // Run the runner as a CLI command with the test output file name and basePath + execSync( + `npx tsx npm/runners/build-post-list-runner.ts --outputFile ${outputFileName} --basePath "${tempDir}"`, + { stdio: 'inherit' } + );Replying to “Can’t we directly run npm scripts?”: Running
npm run build:pagesalso formats MDX (format:mdx) and may alter repo files. For tests, a CLI that only performs the minimal action (or a programmatic call) is safer and faster. If preferred, add a dedicatedbuild:pages:ciscript that runs only the runner and consume that here.
62-71: Assertions assume data from the real pages dir; will break once basePath is honoredThe test expects
Welcomeat/docsandAboutat/aboutregardless of thebasePathcontent. After fixing the runner to actually use--basePathfor scanning, ensure the temp dir contains the expected markdown/pages fixtures (or assert schema/shape rather than exact entries).package.json (2)
10-15: Ensure coverage excludes integration tests consistently
npm run test:coverageuses jest.config, which currently still matches integration tests. Either adjust jest.config (preferred; see comment there) or add ignore flags here too to avoid accidentally running integration tests during coverage.Option A — rely on config (after adding
testPathIgnorePatternsin jest.config): no change needed here.Option B — enforce via script:
- "test:coverage": "jest --coverage", + "test:coverage": "jest --coverage --testPathIgnorePatterns=\"tests/integration|netlify\"",
12-14: Redundant testMatch override in test:unit; safe but can drift from jest.configOverriding
--testMatchand--testPathIgnorePatternsinline is okay but duplicates config. Prefer a single source of truth in jest.config to prevent future drift.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
jest.config.js(1 hunks)npm/runners/build-pages-runner.ts(1 hunks)npm/runners/build-post-list-runner.ts(1 hunks)package.json(1 hunks)tests/integration/build-post-list.int.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: akshatnema
PR: asyncapi/website#3101
File: tests/build-rss.test.js:25-27
Timestamp: 2024-11-01T09:55:20.531Z
Learning: In `tests/build-rss.test.js`, replacing `jest.resetModules()` with `jest.resetAllMocks()` in the `afterEach()` block causes errors. It is necessary to use `jest.resetModules()` to reset the module registry between tests in this file.
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is enforced by the project's configuration which uses `moduleResolution: "bundler"` in tsconfig.json and TypeScript-aware ESLint plugins. The .ts extensions are required even though the files are imported using CommonJS require statements.
Applied to files:
jest.config.js
📚 Learning: 2024-12-30T11:00:42.064Z
Learnt from: JeelRajodiya
PR: asyncapi/website#3423
File: jest.config.cjs:6-7
Timestamp: 2024-12-30T11:00:42.064Z
Learning: The user only wants coverage for scripts, not for .tsx files, because the existing tests are focused on scripts.
Applied to files:
jest.config.js
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
PR: asyncapi/website#3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In the AsyncAPI website project, JavaScript test files must include the .ts extension when importing TypeScript files (e.g., `require('../scripts/build-rss.ts')`). This is a project-specific requirement enforced by the linting rules and build setup, even though it differs from typical Node.js behavior.
Applied to files:
jest.config.js
📚 Learning: 2024-11-01T09:35:23.912Z
Learnt from: akshatnema
PR: asyncapi/website#3101
File: scripts/build-rss.js:4-6
Timestamp: 2024-11-01T09:35:23.912Z
Learning: Converting `getAllPosts()` to asynchronous in `scripts/build-rss.js` causes the existing build system to fail.
Applied to files:
npm/runners/build-post-list-runner.ts
📚 Learning: 2024-11-10T18:16:22.273Z
Learnt from: vishvamsinh28
PR: asyncapi/website#3284
File: scripts/build-post-list.js:49-53
Timestamp: 2024-11-10T18:16:22.273Z
Learning: In `scripts/build-post-list.js`, the `walkDirectories` function depends on sequential execution because it mutates `resultObj`. Parallelizing the loop with `Promise.all` causes tests to fail.
Applied to files:
npm/runners/build-post-list-runner.tstests/integration/build-post-list.int.test.ts
📚 Learning: 2025-06-19T13:51:27.459Z
Learnt from: sagarkori143
PR: asyncapi/website#4192
File: npm/runners/build-pages-runner.ts:7-14
Timestamp: 2025-06-19T13:51:27.459Z
Learning: In the AsyncAPI website build system, individual runner functions use try-catch blocks that rethrow errors to maintain consistent error boundaries, while the top-level orchestrator in npm/index.ts provides meaningful error logging with task-specific context using logger.error(`Error building ${taskName}:`, error). This separation of concerns allows for clean error handling at different architectural layers.
Applied to files:
npm/runners/build-post-list-runner.ts
🔇 Additional comments (2)
package.json (2)
10-15: Script flow LGTM: unit then integrationRunning unit first and integration next keeps feedback quick and isolates failures. Nice.
8-43: ✅ e2e script removal verified safeNo references to the removed
test:e2escript were found in CI workflows, documentation, or dev tooling after searching:
- CI configs (
.github/**/*.yml) contain notest:e2eor “e2e” job definitions- Documentation files (
*.md,*.yml) make no calls tonpm run test:e2e- No tooling or scripts invoke Cypress via an
e2escriptRemoval is intentional and safe to merge.
| // Extract the file name and basePath from the CLI command | ||
| const outputFileArgIndex = process.argv.indexOf('--outputFile'); | ||
| const outputFileName = outputFileArgIndex === -1 ? 'posts.json' : process.argv[outputFileArgIndex + 1]; | ||
|
|
||
| const basePathArgIndex = process.argv.indexOf('--basePath'); | ||
| const basePath = basePathArgIndex === -1 ? undefined : process.argv[basePathArgIndex + 1]; | ||
|
|
||
| // Build outputPath using resolve(basePath, outputFileName) | ||
| let outputPath; | ||
|
|
||
| if (basePath) { | ||
| outputPath = resolve(basePath, outputFileName); | ||
| } else { | ||
| outputPath = resolve(currentDirPath, '../../config', outputFileName); | ||
| } | ||
|
|
||
| await runBuildPostList({ outputPath }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
CLI --basePath is not forwarded to the runner; option currently affects only output path
You parse --basePath, but you don’t pass it into runBuildPostList. As a result, scanning still uses the default ../../pages, while only the output file is written under the provided base path. This is surprising and will bite consumers expecting basePath to control the scan root.
Apply this diff to forward basePath and make arg parsing more robust:
- // Extract the file name and basePath from the CLI command
- const outputFileArgIndex = process.argv.indexOf('--outputFile');
- const outputFileName = outputFileArgIndex === -1 ? 'posts.json' : process.argv[outputFileArgIndex + 1];
-
- const basePathArgIndex = process.argv.indexOf('--basePath');
- const basePath = basePathArgIndex === -1 ? undefined : process.argv[basePathArgIndex + 1];
-
- // Build outputPath using resolve(basePath, outputFileName)
- let outputPath;
-
- if (basePath) {
- outputPath = resolve(basePath, outputFileName);
- } else {
- outputPath = resolve(currentDirPath, '../../config', outputFileName);
- }
-
- await runBuildPostList({ outputPath });
+ // Extract CLI args safely
+ const { values } = parseArgs({
+ options: {
+ outputFile: { type: 'string' },
+ basePath: { type: 'string' }
+ }
+ });
+ const outputFileName = values.outputFile || 'posts.json';
+ const basePath = values.basePath;
+
+ // Build outputPath using resolve(basePath, outputFileName)
+ const outputPath = basePath
+ ? resolve(basePath, outputFileName)
+ : resolve(currentDirPath, '../../config', outputFileName);
+
+ await runBuildPostList({ outputPath, basePath });Add this import at the top of the file:
import { parseArgs } from 'node:util';Follow-up: with this change, the integration test will point the scan to the temp dir. To keep the test green, either:
- Generate pages into that same temp dir (preferable; see suggested change in build-pages-runner), or
- Omit
--basePathin the test until build-pages-runner accepts a target override.
🤖 Prompt for AI Agents
In npm/runners/build-post-list-runner.ts around lines 56-72, the CLI --basePath
is parsed but not forwarded to the runner and the arg parsing should be made
more robust; add an import for parseArgs from 'node:util', use parseArgs to
reliably extract --basePath and --outputFile (with sensible defaults), build
outputPath as before, and pass the basePath into the runBuildPostList invocation
(e.g. include basePath in the options object) so the scanner uses the provided
base path.
2 participants
Description
Summary by CodeRabbit