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 WalkthroughExports many runner interfaces/functions, tightens CLI execution guards (module vs test), changes scripts API (adopters), centralizes test wiring in package.json, adds extensive integration tests, test fixtures/helpers, and updates several config JSON/data files. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as Runner module (CLI)
participant MOD as Module import (tests)
participant FS as File System
participant TEST as Test code
CLI->>CLI: module executed directly (import.meta.url / process checks)
CLI->>CLI: parse CLI args (--outputFile/--basePath)
CLI->>FS: runBuildX(options) reads/writes files
CLI-->>CLI: exit(0) / exit(1) on error
Note right of MOD: when imported (tests)
TEST->>MOD: import runBuildX
TEST->>MOD: call runBuildX({ ... })
MOD->>FS: perform operations, return Promise
TEST-->>MOD: assert outputs
sequenceDiagram
autonumber
participant Pages as build-pages-runner
participant PostList as build-post-list-runner
participant Temp as Temp dir
participant FS as File System
Temp->>Pages: provide basePath
Pages->>FS: generate pages into basePath
Pages-->>Temp: pages created
Temp->>PostList: call with basePath + outputPath
PostList->>FS: read pages, write posts.json to outputPath
PostList-->>Temp: posts.json created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Potential hotspots to inspect:
Possibly related PRs
Suggested reviewers
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 |
sagarkori143
changed the title
…ges directory for building posts
sagarkori143
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## add-integration-tests #4353 +/- ##
==========================================================
- Coverage 100.00% 55.25% -44.75%
==========================================================
Files 21 21
Lines 828 838 +10
Branches 165 169 +4
==========================================================
- Hits 828 463 -365
- Misses 0 340 +340
- Partials 0 35 +35 ☔ 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.
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.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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)
config/all-tags.json (1)
94-325: Remove duplicate entries in the technologies array to maintain data integrity.Verification confirms 4 technologies have duplicate entries that should be deduplicated:
- "AsyncAPI Generator" (2 occurrences)
- "HTML" (2 occurrences)
- "SmartPaste" (2 occurrences)
- "VSCode" (2 occurrences)
These duplicates create unnecessary bloat, complicate lookups, and should be consolidated.
♻️ Duplicate comments (1)
jest.config.js (1)
16-20: Comment and behavior diverge: integration tests are still matched by Jest configThe comment says "except integration," but
testMatchstill includestests/integration. WithouttestPathIgnorePatterns, integration tests will run in commands that rely solely on jest.config (e.g.,npm run test:coverage).Apply this diff to align config with intent:
// Default: all tests except integration testMatch: [ '**/tests/**/*.test.*', '!**/netlify/**/*.test.*' ], + testPathIgnorePatterns: [ + '<rootDir>/tests/integration/', + '<rootDir>/netlify/' + ],
🧹 Nitpick comments (15)
config/meetings.json (1)
17-17: Remove trailing whitespace from title.Line 17 has a trailing space in the title:
"AsyncAPI Design WG Meeting ". This should be trimmed for data consistency.- "title": "AsyncAPI Design WG Meeting ", + "title": "AsyncAPI Design WG Meeting",tests/check-locales.test.ts (1)
509-557: Consider using specific numeric assertions for better test clarity.The test correctly verifies the presence of the
validationSummaryobject structure. However, usingexpect.any(Number)fortotalFilesandfilesWithIssuesmakes the test less precise. Based on your mock setup, you can predict the exact values:
totalFilesshould be2(common.json and file2.json)filesWithIssuesshould be1(only common.json has missing keys)More specific assertions would make test failures easier to diagnose and serve as better documentation of expected behavior.
Example refactor:
validationSummary: expect.objectContaining({ - totalFiles: expect.any(Number), + totalFiles: 2, totalLanguages: 2, - filesWithIssues: expect.any(Number) + filesWithIssues: 1 }),tests/markdown/check-edit-links.test.ts (2)
209-216: Test doesn't verify AbortController signal propagation.The test aborts the controller and mocks fetch to reject, but it doesn't verify that the
signalis actually passed to thefetchcall or that the abort is properly integrated. Consider enhancing this test to verify the signal is passed to fetch:it('should handle AbortController signal', async () => { const controller = new AbortController(); + let receivedSignal = null; controller.abort(); - mockFetch.mockImplementation(() => Promise.reject(new Error('Aborted'))); + mockFetch.mockImplementation((url, options) => { + receivedSignal = options?.signal; + return Promise.reject(new Error('Aborted')); + }); await expect(processBatch(testBatch)).rejects.toThrow(); + expect(receivedSignal).toBe(controller.signal); });
284-306: Consider using afterEach for environment variable cleanup.The test properly saves and restores the environment variable, but the cleanup happens at the end of the test. If an assertion fails before the cleanup code, the environment variable won't be restored. Consider using
afterEachfor more robust cleanup:+describe('checkUrls with environment variable', () => { + let originalEnv: string | undefined; + + beforeEach(() => { + originalEnv = process.env.DOCS_LINK_CHECK_BATCH_SIZE; + }); + + afterEach(() => { + if (originalEnv) { + process.env.DOCS_LINK_CHECK_BATCH_SIZE = originalEnv; + } else { + delete process.env.DOCS_LINK_CHECK_BATCH_SIZE; + } + }); + it('should use DOCS_LINK_CHECK_BATCH_SIZE environment variable', async () => { - const originalEnv = process.env.DOCS_LINK_CHECK_BATCH_SIZE; - process.env.DOCS_LINK_CHECK_BATCH_SIZE = '2'; mockFetch.mockImplementation(() => Promise.resolve({ status: 200 })); const largePathsArray = Array.from({ length: 10 }, (_, i) => ({ filePath: `test${i}.md`, urlPath: `test${i}`, editLink: `https://example.com/test${i}` })); await checkUrls(largePathsArray); // Should create 5 batches (10 items / 2 per batch) expect(logger.info).toHaveBeenCalledWith('Processing 5 batches concurrently...'); - - if (originalEnv) { - process.env.DOCS_LINK_CHECK_BATCH_SIZE = originalEnv; - } else { - delete process.env.DOCS_LINK_CHECK_BATCH_SIZE; - } }); +});tests/integration/case-studies-runner.int.test.ts (1)
264-285: Consider simplifying the error handling test pattern.The current try-catch pattern is functional but convoluted. Consider using a more direct approach:
- it('wraps errors in CustomError with proper context', async () => { - const nonExistentDir = resolve(tempDir, 'non-existent-dir'); - - try { - await runCaseStudies({ - caseStudyDirectory: nonExistentDir, - outputPath - }); - throw new Error('Expected error to be thrown'); - } catch (error) { - if (error instanceof Error && error.message === 'Expected error to be thrown') { - throw error; - } - expect(error).toBeInstanceOf(CustomError); - const customError = error as CustomError; - - expect(customError.context.category).toBe('script'); - expect(customError.context.operation).toBe('runCaseStudies'); - expect(customError.context.detail).toBeDefined(); - expect(customError.context.detail).toContain('Case studies build failed'); - } - }); + it('wraps errors in CustomError with proper context', async () => { + const nonExistentDir = resolve(tempDir, 'non-existent-dir'); + + await expect( + runCaseStudies({ + caseStudyDirectory: nonExistentDir, + outputPath + }) + ).rejects.toThrow(CustomError); + + // Verify context in a separate call + try { + await runCaseStudies({ + caseStudyDirectory: nonExistentDir, + outputPath + }); + } catch (error) { + const customError = error as CustomError; + expect(customError.context.category).toBe('script'); + expect(customError.context.operation).toBe('runCaseStudies'); + expect(customError.context.detail).toBeDefined(); + expect(customError.context.detail).toContain('Case studies build failed'); + } + });tests/integration/helpers/build-pages-lock.ts (2)
42-109: Consider simplifying the nested polling logicThe
waitForPagesBuildfunction has deeply nested loops (4 levels) with multiple await-in-loop patterns. While the linting suppressions acknowledge this, the complexity could be reduced.Consider extracting helper functions:
async function checkPagesDirectory(): Promise<boolean> { try { const pagesDir = resolve(process.cwd(), 'pages'); const stats = await fs.stat(pagesDir); if (!stats.isDirectory()) return false; const topLevelFiles = await fs.readdir(pagesDir); return await hasBuiltSubdirectories(pagesDir, topLevelFiles); } catch { return false; } } async function hasBuiltSubdirectories(pagesDir: string, topLevelFiles: string[]): Promise<boolean> { const requiredDirs = ['blog', 'docs', 'about']; const existingDirs = topLevelFiles.filter(f => requiredDirs.includes(f)); for (const subdir of existingDirs) { if (await hasConvertedFiles(resolve(pagesDir, subdir))) { return true; } } return false; } async function hasConvertedFiles(subdirPath: string): Promise<boolean> { try { const stats = await fs.stat(subdirPath); if (!stats.isDirectory()) return false; const files = await fs.readdir(subdirPath); return files.some(file => file.endsWith('.mdx')); } catch { return false; } }Then simplify the main loop:
export async function waitForPagesBuild(maxWaitMs = 60000): Promise<boolean> { const startTime = Date.now(); const checkInterval = 1000; while (Date.now() - startTime < maxWaitMs) { if (await checkPagesDirectory()) { return true; } // Wait before next check await new Promise(res => setTimeout(res, checkInterval)); } return false; }
29-35: Broad error suppression may mask real issuesAll error handling uses empty catch blocks, which silently suppresses all errors including unexpected ones. While this is acceptable for ENOENT (file not found), other errors like EACCES (permission denied) might indicate real problems.
Consider logging unexpected errors or at least filtering by error code:
} catch (error) { // Expected: lock doesn't exist yet if (error.code !== 'ENOENT') { console.warn('Unexpected error checking lock:', error); } }Also applies to: 80-82, 86-88, 99-104
npm/runners/build-post-list-runner.ts (1)
58-74: Use parseArgs for more robust CLI argument parsingThe manual argument parsing using
indexOfis brittle and doesn't handle edge cases like missing values, duplicate flags, or malformed input.Apply this diff:
+ import { parseArgs } from 'node:util'; + 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); - } + const { values } = parseArgs({ + options: { + outputFile: { type: 'string' }, + basePath: { type: 'string' } + } + }); + + const outputFileName = values.outputFile || 'posts.json'; + const basePath = values.basePath; + + const outputPath = basePath + ? resolve(basePath, outputFileName) + : resolve(currentDirPath, '../../config', outputFileName);Add the import at the top:
import { parseArgs } from 'node:util';scripts/adopters/index.ts (1)
15-51: Error handling is solid; consider tightening context typing.The validation and contextual wrapping around
writeJSONlook good and provide useful metadata (operation,stage,sourcePath,targetPath, truncated stack). The repeated(error as any).contextcasts, though, lose type safety. Consider introducing a smallContextualErrortype or helper (e.g.,withContext(error, context)) so callers can rely on a typedcontextfield withoutanycasts.tests/integration/build-adopters-list-runner.int.test.ts (1)
143-165: Data-quality assertions are strong but may be brittle to future content changes.Enforcing uniqueness of
(companyName|useCase)combinations and resource links at the integration-test level will cause test failures whenever content editors intentionally introduce duplicates (e.g., same use case across teams or shared links). If those invariants are business-critical, this is fine; otherwise, consider softening these checks or moving them to a separate “data lint” suite so normal content updates don’t unnecessarily break CI.tests/integration/build-tools.int.test.ts (1)
29-41: Preserve and restore originalGITHUB_TOKENinstead of unconditionally overwriting/deleting.The suite sets
process.env.GITHUB_TOKEN = 'test-token'inbeforeAlland deletes it inafterAll, which can clobber any real token present when tests start. Capture the original value and restore it, deleting only if it was initially undefined, e.g.:let tempDir: string; let automatedToolsPath: string; let manualToolsPath: string; let toolsPath: string; let tagsPath: string; + let originalGitHubToken: string | undefined; beforeAll(async () => { - // Mock the GITHUB_TOKEN environment variable - process.env.GITHUB_TOKEN = 'test-token'; + // Mock the GITHUB_TOKEN environment variable, preserving original + originalGitHubToken = process.env.GITHUB_TOKEN; + process.env.GITHUB_TOKEN = 'test-token'; @@ afterAll(async () => { @@ - // Clean up environment variable - delete process.env.GITHUB_TOKEN; + // Restore original environment variable + if (originalGitHubToken === undefined) { + delete process.env.GITHUB_TOKEN; + } else { + process.env.GITHUB_TOKEN = originalGitHubToken; + }This keeps the tests isolated from the caller’s environment.
Also applies to: 66-75
tests/integration/build-newsroom-videos-runner.int.test.ts (1)
15-24: Make YOUTUBE_TOKEN restoration robust when no original token exists.You correctly snapshot
originalEnv.YOUTUBE_TOKENbefore overriding, butafterAllonly restores it if the original value is truthy. If there was no token initially, the suite leavesYOUTUBE_TOKEN = 'test-token'set after tests. Consider:afterAll(() => { - // Restore original environment variables - if (originalEnv.YOUTUBE_TOKEN) { - process.env.YOUTUBE_TOKEN = originalEnv.YOUTUBE_TOKEN; - } + // Restore original environment variables + if (typeof originalEnv.YOUTUBE_TOKEN === 'undefined') { + delete process.env.YOUTUBE_TOKEN; + } else { + process.env.YOUTUBE_TOKEN = originalEnv.YOUTUBE_TOKEN; + } });This restores the environment to its exact pre-test state.
Also applies to: 35-40
tests/integration/build-meetings-runner.int.test.ts (1)
195-201: Avoid invoking the function to verify it's async.Calling
runBuildMeetings()without arguments on line 199 actually executes the function, which may trigger side effects (environment variable checks, file operations, API calls) in an uncontrolled test context. Instead, verify the function signature and async nature through type checking or by calling it with proper arguments and asserting it returns a Promise.Consider this safer approach:
describe('Export and Type Safety', () => { it('should export the runBuildMeetings function', () => { expect(typeof runBuildMeetings).toBe('function'); - // Verify it's a Promise-returning function - expect(runBuildMeetings()).toBeInstanceOf(Promise); + // Verify it returns a Promise when called with valid options + const result = runBuildMeetings({ outputPath }); + expect(result).toBeInstanceOf(Promise); + // Clean up - await the promise to prevent unhandled rejection + return result; }); });tests/integration/build-post-list.int.test.ts (1)
16-58: Consider refactoring for improved readability.The
isPagesValidfunction has deep nesting (4 levels) with try-catch blocks inside loops, requiring multiple eslint-disable comments. While functionally correct, extracting the inner logic would improve readability.Consider this refactor:
async function isPagesValid(): Promise<boolean> { try { const pagesDir = resolve(process.cwd(), 'pages'); const stats = await fs.stat(pagesDir); if (!stats.isDirectory()) { return false; } const files = await fs.readdir(pagesDir); const hasExpectedDirs = ['docs', 'blog', 'about'].some(dir => files.includes(dir)); if (!hasExpectedDirs) { return false; } return await hasAnyMdxFiles(pagesDir, ['blog', 'docs', 'about']); } catch { return false; } } async function hasAnyMdxFiles(baseDir: string, subdirs: string[]): Promise<boolean> { for (const subdir of subdirs) { try { const subdirPath = resolve(baseDir, subdir); const subdirStats = await fs.stat(subdirPath); if (subdirStats.isDirectory()) { const subdirFiles = await fs.readdir(subdirPath); if (subdirFiles.some(file => file.endsWith('.mdx'))) { return true; } } } catch { // Subdirectory doesn't exist, continue } } return false; }tests/integration/build-pages-runner.int.test.ts (1)
402-408: Avoid invoking the function to verify it's async.Calling
runBuildPages()without arguments on line 406 actually executes the function, which may trigger file operations and transformations in an uncontrolled test context. This is the same pattern flagged in the build-meetings-runner test.Use a safer approach:
describe('Function Export', () => { it('should export the runner function', () => { expect(typeof runBuildPages).toBe('function'); - // Verify it's a Promise-returning function - expect(runBuildPages()).toBeInstanceOf(Promise); + // Verify it returns a Promise when called with valid options + const result = runBuildPages({ sourceDir, targetDir }); + expect(result).toBeInstanceOf(Promise); + // Await to prevent unhandled rejection + return result; }); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
-
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (36)
-
config/all-tags.json(2 hunks) -
config/meetings.json(1 hunks) -
config/newsroom_videos.json(1 hunks) -
jest.config.js(1 hunks) -
npm/runners/build-adopters-list-runner.ts(2 hunks) -
npm/runners/build-dashboard-runner.ts(3 hunks) -
npm/runners/build-meetings-runner.ts(2 hunks) -
npm/runners/build-newsroom-videos-runner.ts(2 hunks) -
npm/runners/build-pages-runner.ts(2 hunks) -
npm/runners/build-post-list-runner.ts(3 hunks) -
npm/runners/build-tools-runner.ts(1 hunks) -
npm/runners/case-studies-runner.ts(1 hunks) -
package.json(2 hunks) -
scripts/adopters/index.ts(1 hunks) -
scripts/index.ts(1 hunks) -
tests/adopters/index.test.ts(1 hunks) -
tests/build-meetings.test.ts(2 hunks) -
tests/build-rss.test.ts(4 hunks) -
tests/check-locales.test.ts(1 hunks) -
tests/index.test.ts(1 hunks) -
tests/integration/build-adopters-list-runner.int.test.ts(1 hunks) -
tests/integration/build-dashboard.int.test.ts(1 hunks) -
tests/integration/build-finance-info-list-runner.int.test.ts(1 hunks) -
tests/integration/build-meetings-runner.int.test.ts(1 hunks) -
tests/integration/build-newsroom-videos-runner.int.test.ts(1 hunks) -
tests/integration/build-pages-runner.int.test.ts(1 hunks) -
tests/integration/build-post-list.int.test.ts(1 hunks) -
tests/integration/build-tools.int.test.ts(1 hunks) -
tests/integration/case-studies-runner.int.test.ts(1 hunks) -
tests/integration/compose-blog-runner.int.test.ts(1 hunks) -
tests/integration/fixtures/dashboard-fixtures.ts(1 hunks) -
tests/integration/fixtures/nock-helpers.ts(1 hunks) -
tests/integration/fixtures/tools-fixtures.ts(1 hunks) -
tests/integration/helpers/build-pages-lock.ts(1 hunks) -
tests/integration/helpers/file-utils.ts(1 hunks) -
tests/markdown/check-edit-links.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (27)
📚 Learning: 2024-11-01T09:55:20.531Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 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.
Applied to files:
jest.config.jstests/build-rss.test.tstests/integration/build-meetings-runner.int.test.tstests/markdown/check-edit-links.test.tstests/adopters/index.test.tstests/integration/build-dashboard.int.test.tstests/integration/build-tools.int.test.tstests/build-meetings.test.tstests/integration/build-pages-runner.int.test.tstests/integration/build-newsroom-videos-runner.int.test.ts
📚 Learning: 2025-01-03T08:14:02.138Z
Learnt from: JeelRajodiya
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-03T08:14:02.138Z
Learning: The user (JeelRajodiya) has stated a preference to keep the test files in CommonJS rather than migrating them to TypeScript.
Applied to files:
jest.config.js
📚 Learning: 2024-12-30T11:00:42.064Z
Learnt from: JeelRajodiya
Repo: asyncapi/website PR: 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.jstests/integration/build-post-list.int.test.tspackage.json
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 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.jstests/build-rss.test.tsnpm/runners/build-pages-runner.tstests/integration/case-studies-runner.int.test.tstests/adopters/index.test.tstests/integration/compose-blog-runner.int.test.tstests/build-meetings.test.tstests/integration/helpers/build-pages-lock.tstests/integration/helpers/file-utils.tstests/integration/build-adopters-list-runner.int.test.tstests/integration/build-pages-runner.int.test.tstests/integration/build-newsroom-videos-runner.int.test.tspackage.json
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 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.jsnpm/runners/build-pages-runner.tstests/integration/case-studies-runner.int.test.tstests/adopters/index.test.tstests/build-meetings.test.tstests/integration/helpers/build-pages-lock.tstests/integration/build-adopters-list-runner.int.test.tstests/integration/build-pages-runner.int.test.tstests/integration/build-newsroom-videos-runner.int.test.ts
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In JavaScript test files within the AsyncAPI website project, TypeScript file imports must include the .ts extension to avoid lint errors, even though the files being imported are JavaScript files.
Applied to files:
jest.config.js
📚 Learning: 2024-11-01T13:32:15.472Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3101
File: tests/fixtures/rssData.js:1-57
Timestamp: 2024-11-01T13:32:15.472Z
Learning: In the `tests/fixtures/rssData.js` file of the `asyncapi/website` project, tests for edge cases such as empty strings for title or excerpt, very long text content, international characters (UTF-8), or malformed URLs in `slug` or `cover` are not necessary because these cases will not occur.
Applied to files:
tests/build-rss.test.tstests/markdown/check-edit-links.test.tstests/integration/case-studies-runner.int.test.tstests/integration/build-dashboard.int.test.tstests/integration/fixtures/tools-fixtures.tstests/integration/build-tools.int.test.tstests/integration/compose-blog-runner.int.test.tstests/integration/build-finance-info-list-runner.int.test.tstests/integration/build-adopters-list-runner.int.test.tstests/integration/build-post-list.int.test.tstests/integration/build-pages-runner.int.test.tstests/integration/build-newsroom-videos-runner.int.test.tstests/integration/fixtures/dashboard-fixtures.ts
📚 Learning: 2024-11-01T09:35:23.912Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 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:
tests/build-rss.test.tstests/integration/build-meetings-runner.int.test.tsnpm/runners/build-post-list-runner.tsscripts/index.tstests/integration/build-post-list.int.test.ts
📚 Learning: 2024-11-29T17:36:09.783Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3378
File: tests/markdown/check-markdown.test.js:133-138
Timestamp: 2024-11-29T17:36:09.783Z
Learning: When testing the 'main' function in 'check-markdown.test.js', it's acceptable to simply ensure it doesn't throw any errors, as the functions it calls are already tested elsewhere.
Applied to files:
tests/build-rss.test.tstests/integration/build-meetings-runner.int.test.tstests/index.test.tstests/markdown/check-edit-links.test.tstests/integration/case-studies-runner.int.test.tstests/adopters/index.test.tstests/integration/build-dashboard.int.test.tstests/integration/build-tools.int.test.tstests/integration/compose-blog-runner.int.test.tstests/integration/build-adopters-list-runner.int.test.tstests/integration/build-post-list.int.test.tstests/check-locales.test.tstests/integration/build-pages-runner.int.test.tstests/integration/build-newsroom-videos-runner.int.test.ts
📚 Learning: 2025-01-19T04:51:41.255Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3557
File: tests/fixtures/markdown/check-edit-links-data.js:3-11
Timestamp: 2025-01-19T04:51:41.255Z
Learning: In the AsyncAPI website repository, the test data in `tests/fixtures/markdown/check-edit-links-data.js` intentionally includes inconsistent paths (with and without 'docs' prefix) to verify the script's ability to normalize and handle ambiguous path structures.
Applied to files:
tests/build-rss.test.tstests/markdown/check-edit-links.test.tstests/adopters/index.test.tstests/integration/build-dashboard.int.test.tstests/integration/fixtures/tools-fixtures.tstests/integration/build-tools.int.test.tstests/integration/compose-blog-runner.int.test.tstests/integration/build-post-list.int.test.tstests/integration/build-pages-runner.int.test.tstests/integration/fixtures/dashboard-fixtures.ts
📚 Learning: 2024-12-02T05:52:49.547Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3265
File: tests/helper/toolsObjectData.js:1-18
Timestamp: 2024-12-02T05:52:49.547Z
Learning: In `tests/helper/toolsObjectData.js`, using the hard-coded repository ID directly in the URL is acceptable when creating mock data for tests, as we are not asserting anything at `REPO_ID`.
Applied to files:
tests/build-rss.test.tstests/integration/fixtures/tools-fixtures.tstests/integration/build-tools.int.test.tstests/integration/fixtures/dashboard-fixtures.ts
📚 Learning: 2025-06-20T14:47:22.389Z
Learnt from: sagarkori143
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-06-20T14:47:22.389Z
Learning: Runner files in the npm/runners directory should follow consistent error handling patterns, throwing new Error instances with descriptive messages rather than just rethrowing caught errors.
Applied to files:
tests/build-rss.test.tstests/integration/build-meetings-runner.int.test.tstests/markdown/check-edit-links.test.tstests/integration/case-studies-runner.int.test.tstests/integration/build-tools.int.test.tstests/integration/build-adopters-list-runner.int.test.tstests/integration/build-post-list.int.test.tstests/integration/build-pages-runner.int.test.tstests/integration/build-newsroom-videos-runner.int.test.tsnpm/runners/build-adopters-list-runner.ts
📚 Learning: 2024-10-18T17:24:45.053Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-10-18T17:24:45.053Z
Learning: In `scripts/markdown/check-markdown.js`, the script is not run in an asynchronous environment, so refactoring it to use `async/await` is unnecessary.
Applied to files:
npm/runners/build-pages-runner.tstests/markdown/check-edit-links.test.ts
📚 Learning: 2024-11-10T18:16:22.273Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 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-pages-runner.tstests/integration/build-meetings-runner.int.test.tsscripts/adopters/index.tstests/index.test.tstests/markdown/check-edit-links.test.tstests/integration/case-studies-runner.int.test.tstests/adopters/index.test.tstests/integration/compose-blog-runner.int.test.tstests/integration/build-finance-info-list-runner.int.test.tstests/build-meetings.test.tsnpm/runners/build-post-list-runner.tsscripts/index.tstests/integration/helpers/build-pages-lock.tstests/integration/build-adopters-list-runner.int.test.tstests/integration/build-post-list.int.test.tstests/integration/build-pages-runner.int.test.tstests/integration/build-newsroom-videos-runner.int.test.tsnpm/runners/build-adopters-list-runner.ts
📚 Learning: 2025-06-20T14:47:22.389Z
Learnt from: sagarkori143
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-06-20T14:47:22.389Z
Learning: The AsyncAPI website modularization PR uses Promise.allSettled for parallel execution of build tasks, which provides better performance and fault isolation compared to sequential execution.
Applied to files:
tests/integration/build-meetings-runner.int.test.tsnpm/runners/build-post-list-runner.tsscripts/index.tstests/integration/helpers/build-pages-lock.tstests/integration/build-newsroom-videos-runner.int.test.ts
📚 Learning: 2025-06-19T13:51:27.459Z
Learnt from: sagarkori143
Repo: asyncapi/website PR: 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:
tests/integration/build-meetings-runner.int.test.tstests/integration/build-dashboard.int.test.tstests/integration/build-tools.int.test.tsnpm/runners/build-post-list-runner.tstests/integration/build-adopters-list-runner.int.test.tstests/integration/build-pages-runner.int.test.tstests/integration/build-newsroom-videos-runner.int.test.tsnpm/runners/build-dashboard-runner.tsnpm/runners/build-adopters-list-runner.ts
📚 Learning: 2025-06-19T13:49:29.796Z
Learnt from: sagarkori143
Repo: asyncapi/website PR: 4192
File: npm/runners/build-newsroom-videos-runner.ts:8-15
Timestamp: 2025-06-19T13:49:29.796Z
Learning: In the AsyncAPI website modularization project, error handling is centralized in the top-level orchestrator function (npm/index.ts) with comprehensive logging and context. Individual runner functions in npm/runners/ are kept simple and let errors propagate naturally to the centralized handler, avoiding redundant try-catch blocks that only rethrow errors.
Applied to files:
tests/integration/build-meetings-runner.int.test.tstests/integration/build-tools.int.test.tsnpm/runners/case-studies-runner.ts
📚 Learning: 2025-01-14T15:12:29.840Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3557
File: scripts/markdown/check-edit-links.js:83-88
Timestamp: 2025-01-14T15:12:29.840Z
Learning: In the AsyncAPI website's edit link generation logic, the `editUrls` configuration (config/edit-page-config.json) includes a fallback entry with an empty value that acts as a catch-all matcher. Therefore, `editOptions.find()` will always return a match, making null checks on the result unnecessary and untestable.
Applied to files:
tests/markdown/check-edit-links.test.ts
📚 Learning: 2025-01-08T15:15:00.759Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3557
File: scripts/markdown/check-editlinks.js:58-59
Timestamp: 2025-01-08T15:15:00.759Z
Learning: In the AsyncAPI codebase, batch processing operations (like in the Dashboard script and check-editlinks.js) follow a sequential pattern using await in loops, which is the preferred approach for maintaining consistency across the codebase.
Applied to files:
tests/markdown/check-edit-links.test.ts
📚 Learning: 2025-04-20T16:05:16.482Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3950
File: scripts/utils/check-locales.ts:122-129
Timestamp: 2025-04-20T16:05:16.482Z
Learning: In the AsyncAPI website project, Next.js throws errors at runtime when locale files are missing, making additional validation for missing files unnecessary in the check-locales script.
Applied to files:
tests/markdown/check-edit-links.test.tstests/integration/build-post-list.int.test.tstests/check-locales.test.tstests/integration/build-pages-runner.int.test.ts
📚 Learning: 2025-02-16T12:57:24.575Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-16T12:57:24.575Z
Learning: Tests failing after dependency upgrades indicate potential breaking changes and might require code changes rather than test modifications to support newer versions.
Applied to files:
tests/adopters/index.test.tspackage.json
📚 Learning: 2024-11-29T19:42:31.175Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3265
File: scripts/tools/tools-object.js:93-98
Timestamp: 2024-11-29T19:42:31.175Z
Learning: In `scripts/tools/tools-object.js`, when validation fails in the `convertTools` function, errors should be thrown or rejected instead of only logging them, to ensure proper error propagation and handling.
Applied to files:
tests/integration/build-tools.int.test.tstests/check-locales.test.ts
📚 Learning: 2024-10-18T17:28:41.718Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-10-18T17:28:41.718Z
Learning: In `scripts/markdown/check-markdown.js`, when processing large markdown files with recursion and loops, synchronous file reads (`fs.readFileSync`) are preferred over asynchronous reads.
Applied to files:
tests/build-meetings.test.ts
📚 Learning: 2024-10-19T15:26:43.683Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3301
File: scripts/markdown/check-markdown.js:127-128
Timestamp: 2024-10-19T15:26:43.683Z
Learning: In `scripts/markdown/check-markdown.js`, synchronous file reading with `fs.readFileSync` is necessary because the files need to be processed synchronously.
Applied to files:
tests/build-meetings.test.ts
📚 Learning: 2024-11-10T18:16:35.551Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3284
File: scripts/build-post-list.js:28-46
Timestamp: 2024-11-10T18:16:35.551Z
Learning: In the `scripts/build-post-list.js` JavaScript file, tests rely on the `result` object being a global variable. Moving it inside the `buildPostList` function causes tests to fail; therefore, keep `result` as a global variable.
Applied to files:
tests/integration/build-post-list.int.test.ts
📚 Learning: 2024-10-18T17:37:50.602Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3301
File: scripts/markdown/check-markdown.js:31-31
Timestamp: 2024-10-18T17:37:50.602Z
Learning: In `scripts/markdown/check-markdown.js`, the `frontmatter` object cannot have a property named `hasOwnProperty`, so using `frontmatter.hasOwnProperty(attr)` is acceptable.
Applied to files:
tests/integration/build-post-list.int.test.ts
📚 Learning: 2025-03-23T12:42:49.503Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3950
File: scripts/utils/check-locales.ts:138-143
Timestamp: 2025-03-23T12:42:49.503Z
Learning: For the locale validation function in check-locales.ts, the team has decided that modifying the outer scope variable 'hasErrors' inside the forEach loop is acceptable despite the ESLint warning about unsafe references in loop functions.
Applied to files:
tests/check-locales.test.ts
🔇 Additional comments (62)
config/meetings.json (1)
1-107: Data integrity looks good overall.The JSON structure is well-formed across all 15 meeting entries. Calendar event IDs, GitHub issue URLs, and ISO 8601 date formatting are consistent. The only minor issue is the trailing space noted above.
One note: Several entries are dated before the PR creation date (2025-08-24), such as meetings on 2025-08-07 and 2025-08-12. If these represent historical meetings intended for integration test fixtures, verify that the test logic handles past-dated entries appropriately to avoid flaky tests based on current date comparisons.
config/all-tags.json (3)
1-30: AI summary describes changes inconsistently with actual code.The AI summary claims: "Added new public language object in the languages array: { "name": "Go", ... }" However, the "Go" entry (lines 225–229) is in the
technologiesarray, notlanguages. Thelanguagesarray ends at line 92.
225-229: Verify the "Go" entry addition and color assignment.The new "Go" entry uses the same default color scheme (
bg-[#61d0f2]/border-[#40ccf7]) as dozens of other entries. This may be a placeholder. Consider:
- Whether "Go" should have a dedicated, language-specific color (e.g.,
bg-[#00ADD8]for Go's official blue)- Whether this addition is required for the integration tests or represents a separate maintenance task
286-289: Clarify the purpose of the label swap between SmartPaste and JetBrains.Lines 286–289 and 291–294 are marked as changed and appear to swap the names of these two entries:
- Line 281–284: SmartPaste (unchanged)
- Line 286–289: SmartPaste (marked as changed)
- Line 291–294: JetBrains (marked as changed)
This suggests a label swap, but the intent is unclear. Verify whether:
- This is a data correction to fix a misplaced label
- These edits support a specific test fixture or requirement
Also applies to: 291-294
tests/markdown/check-edit-links.test.ts (8)
175-181: LGTM!Good edge case coverage for null editLink handling. The test properly verifies both the return value and that no network call is made.
191-200: LGTM!Good verification of resource cleanup. The test properly ensures that timeout cleanup happens in the finally block.
202-207: LGTM!Good edge case coverage for empty batch handling.
278-282: LGTM!Good edge case coverage for empty input handling.
308-320: LGTM!Good edge case coverage for testing batching logic with non-divisible array sizes.
183-189: Error message format is correct.The implementation at line 61 of
scripts/markdown/check-edit-links.tsthrowsnew Error(Error checking ${editLink}: ${error}), which matches the test expectation of.rejects.toThrow('Error checking'). The test correctly validates that the error message begins with "Error checking".
260-261: Error message is correct.The implementation throws
"Failed to check edit links: ${error}"(line 229), and the test expects'Failed to check edit links'. Jest's.toThrow()performs substring matching, so the expected message correctly matches the error thrown.
112-121: Error message verification confirms both tests are correct. ThegeneratePathsfunction at line 193 throws errors with the formatError processing directory ${folderPath}: ${err}. Both test expectations properly match this behavior:
- Line 112-113: Substring "Error processing directory" matches ✓
- Line 115-121: Substring "Directory read error" matches the re-thrown error ✓
tests/integration/build-finance-info-list-runner.int.test.ts (8)
1-30: LGTM! Well-structured test setup.The imports are appropriate, and the setup/teardown pattern correctly creates isolated temporary directories for each test with proper cleanup.
32-107: LGTM! Comprehensive file creation tests.The tests properly verify JSON file generation from YAML sources and directory creation behavior.
109-251: LGTM! Thorough content validation.The tests comprehensively validate YAML-to-JSON conversion, including nested structures and empty arrays.
253-358: LGTM! Comprehensive year selection tests.The tests properly verify automatic latest year selection, explicit year usage, and correct descending year sorting.
396-424: LGTM! Custom path configuration tested correctly.The test properly verifies that custom directory paths are respected by the runner.
426-520: LGTM! Comprehensive error handling coverage.The tests properly verify all error scenarios including missing data, missing files, and non-numeric directory filtering. The use of
.rejects.toThrow(CustomError)correctly tests async errors.
522-617: LGTM! Excellent edge case coverage.The tests handle special characters, large files (100 entries), and file overwriting scenarios. The YAML generation logic is correct, including proper escaping and formatting.
360-393: Verify test isolation when parallel execution is enabled, or reconfigure Jest to run integration tests sequentially.The test uses
process.chdir()to test default path behavior with proper restoration in the finally block (line 390). However, sincejest.config.jsdoes not disable parallel execution (nomaxWorkersorrunInBandsettings), Jest 29.7.0 will run tests concurrently on multi-core systems. This creates a race condition: if multiple tests changeprocess.cwd()in parallel, the global process state can be corrupted even with individual finally blocks restoring values.Recommended fixes:
- Configure Jest to run integration tests serially: add
maxWorkers: 1for integration tests injest.config.js, or mark this test suite with.serial(if available), OR- Use isolated worker processes that don't share global
process.cwd()state.npm/runners/case-studies-runner.ts (2)
56-67: LGTM! Clean execution guard pattern.The guard correctly prevents execution in test environments using both
NODE_ENVandVITEST_WORKER_IDchecks. The IIFE pattern properly handles top-level async execution, and the error handling aligns with the centralized pattern where detailed logging occurs withinrunCaseStudiesitself.
69-70: LGTM! Export enables testability.The export properly exposes
runCaseStudiesfor integration testing and external usage, aligning with the modularization pattern across other runner files in this PR.tests/integration/case-studies-runner.int.test.ts (3)
14-89: LGTM! Well-structured test setup.The
beforeAllhook properly creates an isolated temporary test environment with diverse YAML fixtures covering multi-line strings, arrays, nested objects, and multiple AsyncAPI versions. TheafterAllcleanup ensures no test artifacts remain.
329-474: Excellent edge case coverage!This test section thoroughly validates minimal fields, full optional fields, multiple files, and different YAML extensions. The comprehensive fixture on lines 362-415 is particularly valuable for ensuring the runner handles complex nested structures correctly.
192-196: No issues found—test will pass reliably with current project setup.Verification confirms the
config/casestudiesdirectory exists with two syntactically valid YAML files (adeo.yml, hdi.yml). TherunCaseStudies()function correctly defaults to this directory and includes explicit validation that throws aCustomErrorif the directory is missing, preventing unpredictable failures. The test is not fragile and will execute successfully.tests/build-rss.test.ts (3)
147-149: LGTM: Consistent multi-line formatting improves readabilityThe jest.doMock calls have been reformatted to split the options object across multiple lines, improving consistency and readability throughout the test file.
Also applies to: 182-184, 206-208, 220-222, 238-238, 258-260
219-244: LGTM: Comprehensive error handling testsThese new tests properly verify CustomError handling—both re-throwing existing CustomError instances without wrapping and wrapping non-Error exceptions. The tests validate error context properties, ensuring robust error propagation.
246-269: LGTM: Good edge case coverage for posts without cover imagesThis test ensures that posts without cover images don't generate
<enclosure>tags in the RSS feed, which is the correct RSS behavior.npm/runners/build-tools-runner.ts (1)
64-79: LGTM: Clean public API and direct-execution guardThe exports enable testing and programmatic usage, while the
import.meta.urlguard ensures the module only auto-runs when executed directly, avoiding side effects on import.tests/index.test.ts (1)
48-100: LGTM: Thorough coverage of finance directory parsingThese new tests comprehensively verify the finance directory handling logic: filtering non-numeric entries, sorting years correctly, handling decimal numbers, and properly throwing errors when no valid data is found.
config/newsroom_videos.json (1)
3-30: LGTM: Updated newsroom video dataThe video entries have been refreshed with current 2025 content. The structure remains consistent, and the data appears valid.
npm/runners/build-pages-runner.ts (1)
5-8: LGTM: Public API exports and safe direct-execution guardThe exported interface and function enable testing and programmatic usage. The
import.meta.urlguard ensures the CLI only runs when executed directly, addressing the concern about side effects on import.Also applies to: 47-61
tests/integration/helpers/build-pages-lock.ts (1)
12-24: LGTM: Atomic lock acquisition using exclusive flagThe use of the
wxflag ensures atomic lock creation, preventing race conditions when multiple test processes attempt to acquire the lock simultaneously.npm/runners/build-post-list-runner.ts (1)
11-15: LGTM: Clean public API with proper error handlingThe exported interface and function enable testing and external usage. Error handling properly wraps errors in CustomError with context.
Also applies to: 28-50
package.json (2)
10-10: LGTM! Test script consolidation aligns with integration test setup.The test script changes properly separate unit and integration test execution, with appropriate path targeting and exclusion patterns.
Also applies to: 13-14
179-179: nock dependency verified as current and secure.The nock version 14.0.10 is the latest available on npm with no known security vulnerabilities. The dependency is appropriately placed in devDependencies for HTTP mocking in tests.
tests/integration/compose-blog-runner.int.test.ts (4)
12-24: LGTM! Robust test setup and cleanup.The beforeEach and afterEach hooks properly manage temporary directories with unique naming and comprehensive cleanup.
115-266: LGTM! Comprehensive content validation coverage.The tests thoroughly validate front matter structure, tag formatting, empty value handling, and default metadata generation.
427-505: LGTM! Thorough error handling validation.The tests properly validate CustomError usage, error contexts, and various failure scenarios including missing fields and file conflicts.
507-714: LGTM! Excellent edge case coverage and API verification.The tests cover various edge cases including special characters, long content, different post types, and properly verify the public API surface.
npm/runners/build-dashboard-runner.ts (2)
11-13: LGTM! Clean public API interface.The exported
BuildDashboardOptionsinterface provides a clear contract for the runner's configuration.
25-67: LGTM! Proper test environment handling and CLI integration.The function export, error handling, and CLI guard properly prevent auto-execution in test environments while maintaining CLI functionality. The past review feedback has been addressed.
Based on learnings
tests/build-meetings.test.ts (2)
1-1: LGTM! Appropriate migration to promise-based cleanup.The import update enables promise-based cleanup while maintaining synchronous operations where appropriate for the test context.
42-49: LGTM! Robust async cleanup with appropriate error handling.The async cleanup with try/catch properly handles potential cleanup failures, especially on Windows where file handle delays can occur. The comment provides valuable context.
tests/integration/helpers/file-utils.ts (1)
10-28: LGTM! Clean recursive file listing implementation.The function properly traverses directories and accumulates file paths. The sequential processing with
no-await-in-loopis appropriate for maintaining order and simplicity, though it could be optimized withPromise.allif performance becomes a concern.npm/runners/build-meetings-runner.ts (2)
11-13: LGTM! Consistent public API exports.The interface and function exports follow the established pattern across runners, enabling both testing and external usage.
Also applies to: 57-57
43-54: LGTM! Proper test environment guard.The CLI execution guard correctly prevents auto-execution in test environments, matching the pattern established in other runners.
tests/adopters/index.test.ts (2)
13-13: LGTM! Tests properly updated for new function signature.The test calls correctly use the new two-argument signature for
buildAdoptersList, maintaining test coverage while adapting to API changes.Also applies to: 20-25
31-37: LGTM! Appropriate input validation test coverage.The new tests properly verify that required parameters are validated with clear error messages.
npm/runners/build-newsroom-videos-runner.ts (2)
11-13: LGTM! Consistent public API pattern.The interface and function exports maintain consistency with other runner modules in the codebase.
Also applies to: 57-57
44-54: LGTM! Proper test environment guard in place.The CLI execution guard correctly prevents auto-execution during test imports, following the established pattern.
scripts/index.ts (1)
36-39: Adopters list invocation correctly updated to new API.Supplying explicit source and target config paths via
resolve(currentDirPath, '../config', ...)matches the newbuildAdoptersList(sourcePath, targetPath)contract and keeps path handling consistent with other builders in this script.tests/integration/build-adopters-list-runner.int.test.ts (1)
13-38: Temp workspace setup and runner invocation look clean.Using a unique
tempDirunderos.tmpdir, wiringsourcePathto the realconfig/adopters.yml, and cleaning up inafterAllgives good isolation while still exercising the real pipeline viarunBuildAdoptersList. No changes needed here.tests/integration/build-tools.int.test.ts (1)
227-262: Good coverage ofCustomErrorcontext for GitHub API failures.The
should handle GitHub API errors gracefullytest validatingCustomError,context.category,context.operation, message contents, andcontext.detailgives solid assurance that API failures propagate with meaningful context, matching the earlier guidance around structured error handling for scripts. Based on learnings.tests/integration/build-meetings-runner.int.test.ts (1)
236-258: LGTM: Environment restoration is properly guarded.The try/finally blocks ensure
CALENDAR_SERVICE_ACCOUNTandCALENDAR_IDare restored even if the test assertions fail, preventing environment pollution across tests.tests/integration/fixtures/tools-fixtures.ts (1)
1-399: LGTM: Comprehensive test fixtures.The mock data is well-structured with diverse scenarios covering GitHub API responses, tool metadata, pagination, manual/automated tools, tags, and error cases. This provides excellent coverage for integration tests.
npm/runners/build-adopters-list-runner.ts (1)
8-59: LGTM: Consistent runner pattern with proper CLI guards.The implementation follows the established pattern for runner modules:
- Configurable options with sensible defaults
- CLI execution guard prevents auto-run in test environments (checking both
NODE_ENVandVITEST_WORKER_ID)- Public export enables integration testing
- CustomError wrapping with contextual details
Based on learnings
tests/integration/build-post-list.int.test.ts (1)
81-374: LGTM: Comprehensive integration test coverage.The test suite thoroughly validates:
- File creation and structure
- Section presence (docs, blog, about, docsTree)
- Required fields and type validation
- Slug uniqueness and prefixes
- TOC structure, reading time, excerpts
- Nested sections, weights, index files
- Error handling for invalid inputs
The 30-second timeout on line 114 appropriately accommodates the build operations.
tests/integration/fixtures/dashboard-fixtures.ts (1)
1-500: LGTM: Well-structured dashboard test fixtures.The mock data comprehensively covers dashboard scenarios including hot discussions (issues and PRs), good first issues, pagination with
hasNextPage, pull requests with reviews, and error responses. The data structure matches the expected GraphQL response format.tests/integration/build-pages-runner.int.test.ts (1)
9-713: LGTM: Excellent integration test coverage.The test suite is exceptionally thorough, covering:
- File creation and structural preservation
.md→.mdxrenaming- Content transformations (HTML comments → JSX, table tag capitalization)
- Default options and error handling
- Edge cases (special characters, unicode, deep nesting, large files)
- Idempotency
The tests validate both the happy path and numerous edge cases, ensuring robust behavior.
tests/integration/build-dashboard.int.test.ts (1)
17-300: LGTM: Comprehensive dashboard integration tests.The test suite provides thorough coverage:
- File creation and structure validation
- CustomError handling with proper context
- Rate limit warnings and pagination
- Pull request handling via
getDiscussionByID- Data integrity (required fields, uniqueness, sorting)
- Missing GitHub token scenario
The use of helper mocks from
nock-helpers.tskeeps the tests clean and maintainable. Proper cleanup inafterEachandafterAllprevents test interference.tests/integration/fixtures/nock-helpers.ts (1)
1-323: LGTM: Well-organized nock helper utilities.The helper functions effectively encapsulate nock setup for different integration test scenarios:
- Dashboard mocks: GraphQL API with pagination, rate limits, and error handling
- Tools mocks: GitHub Code Search and raw content APIs
- Newsroom mocks: YouTube API interactions
The request body parsing logic (lines 73-96, 131-154) properly handles Buffer and string types, making the mocks robust. Clear JSDoc comments document each helper's purpose.
akshat-nema
left a comment
akshat-nema
left a comment
There was a problem hiding this comment.
Why codecov coverage is still around 40% in it?
Co-authored-by: Akshat Nema <158018845+akshat-nema@users.noreply.github.com>
Co-authored-by: Akshat Nema <158018845+akshat-nema@users.noreply.github.com>
Co-authored-by: Akshat Nema <158018845+akshat-nema@users.noreply.github.com>
Co-authored-by: Akshat Nema <158018845+akshat-nema@users.noreply.github.com>
Co-authored-by: Akshat Nema <158018845+akshat-nema@users.noreply.github.com>
Co-authored-by: Akshat Nema <158018845+akshat-nema@users.noreply.github.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 11
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-tools-runner.ts (1)
64-79: Tighten the CLI “run when main” check for cross‑platform reliabilityThe guard
if (import.meta.url === \file://${process.argv[1]}`)` relies on constructing a file URL by string concatenation, which can break on Windows paths and edge cases (spaces, encoding). It’s safer to compare OS paths:-if (import.meta.url === `file://${process.argv[1]}`) { +if (fileURLToPath(import.meta.url) === process.argv[1]) { (async () => { try { await runBuildTools(); } catch (error) { process.exit(1); } })(); }This reuses the existing
fileURLToPathimport and keeps the semantics while being more robust.npm/runners/build-dashboard-runner.ts (1)
11-49: Use the resolvedoutputPathin error context instead ofoptions.outputPath
runBuildDashboard’s error handling is aligned with the runners’ pattern (wrap inCustomError, addoperation/detail, log, rethrow), but the context currently usesoptions.outputPath, which is oftenundefinedwhen the default path is used:const outputPath = options.outputPath || resolve(currentDirPath, '../../dashboard.json'); // ... detail: `Runner failed with output path: ${options.outputPath}`Switching the detail to use the resolved
outputPathwill give accurate diagnostics regardless of whether the caller passed an explicit value:- operation: 'runBuildDashboard', - detail: `Runner failed with output path: ${options.outputPath}` + operation: 'runBuildDashboard', + detail: `Runner failed with output path: ${outputPath}` @@ - category: 'script', - operation: 'runBuildDashboard', - detail: `Build dashboard runner failed with output path: ${options.outputPath}` + category: 'script', + operation: 'runBuildDashboard', + detail: `Build dashboard runner failed with output path: ${outputPath}`Optionally, you may also want to normalize CLI-provided paths through
resolvefor consistency with the default path, but that’s a nice-to-have.
♻️ Duplicate comments (4)
jest.config.js (1)
16-20: Comment and configuration still diverge: integration tests remain matchedThe past review comment correctly identified that the comment claims "except integration" but
testMatchstill includestests/integration. The suggested fix to addtestPathIgnorePatternshas not been applied.npm/runners/build-pages-runner.ts (1)
47-58: Use path comparison instead of URL string comparison in main guardThe direct-run guard currently compares
import.meta.urltofile://${process.argv[1]}, which can be fragile on Windows or with unusual paths/encoding. Replacing it with a path comparison is safer:-if (import.meta.url === `file://${process.argv[1]}`) { +if (fileURLToPath(import.meta.url) === process.argv[1]) { (async () => { try { await runBuildPages(); } catch (error) { process.exit(1); } })(); }This keeps import-time side effects off while improving cross-platform behavior.
npm/runners/build-adopters-list-runner.ts (1)
26-43: Use resolved paths in error detail for clearer diagnostics
runBuildAdoptersListcorrectly wraps failures in aCustomError, but thedetailfield currently usesoptions.sourcePath/options.targetPath. When callers omit options and rely on defaults, this will logundefinedinstead of the actual resolved paths.You can improve the error context by using the computed values:
- const sourcePath = options.sourcePath || resolve(currentDirPath, '../../config', 'adopters.yml'); - const targetPath = options.targetPath || resolve(currentDirPath, '../../config', 'adopters.json'); + const sourcePath = options.sourcePath || resolve(currentDirPath, '../../config', 'adopters.yml'); + const targetPath = options.targetPath || resolve(currentDirPath, '../../config', 'adopters.json'); @@ - detail: `Build adopters list failed - sourcePath: ${options.sourcePath}, targetPath: ${options.targetPath}` + detail: `Build adopters list failed - sourcePath: ${sourcePath}, targetPath: ${targetPath}`This keeps the contract unchanged while making logs and error reports more actionable.
tests/integration/build-post-list.int.test.ts (1)
19-58: AlignwaitForPagesmax wait with Jest'sbeforeAlltimeoutThe concern is valid:
waitForPagesdefaults to 60 000 ms, while thebeforeAllat line 76 has a 30 000 ms timeout. If page builds take between 30–60 seconds, Jest will timeout the hook beforewaitForPagescompletes, causing flaky failures.Additionally, the return value of
waitForPages()at line 105 is not checked—the code proceeds regardless of success or timeout, masking failures.Consider either:
- Lowering
waitForPages's default (or passing a smallermaxWaitMsfrombeforeAll), or- Increasing the hook timeout to at least 60 000 ms
Also validate the boolean result and fail early with a clear error if pages never become valid, rather than letting Jest timeout silently.
🧹 Nitpick comments (25)
tests/integration/case-studies-runner.int.test.ts (1)
495-505: Use custom test directories for better isolation.The promise is now properly awaited (good!), but calling
runCaseStudies()without arguments uses real project directories which could fail unpredictably and isn't isolated. The test should focus on verifying the export and promise behavior using controlled test paths.Apply this diff to use isolated test directories:
describe('Function Export', () => { it('should export the runner function', async () => { expect(typeof runCaseStudies).toBe('function'); - // Verify it's a Promise-returning function - const promise = runCaseStudies(); + }); + + it('should return a Promise', async () => { + const testDir = resolve(tempDir, 'export-test'); + const testOutput = resolve(tempDir, 'export-test-output.json'); + await fs.mkdir(testDir, { recursive: true }); + + const promise = runCaseStudies({ + caseStudyDirectory: testDir, + outputPath: testOutput + }); expect(promise).toBeInstanceOf(Promise); - // Await the Promise to handle any potential rejections await promise; }); });tests/build-rss.test.ts (2)
154-188: Consider verifying the error message content.The test creates detailed mock data with posts missing dates (including specific titles and slugs), and the test name suggests it should verify that these posts are "included in the error message." However, the test only checks that a
CustomErroris thrown without verifying the error message content.Consider adding an assertion to verify the error message contains the expected post identifiers:
- // This should specifically test the error thrown by the missingDatePosts check - await expect(rssFeed(type, title, desc, outputPath)).rejects.toThrow(CustomError); + // This should specifically test the error thrown by the missingDatePosts check + try { + await rssFeed(type, title, desc, outputPath); + } catch (error) { + expect(error).toBeInstanceOf(CustomError); + const errorMessage = (error as CustomError).message; + expect(errorMessage).toContain('Post 1 without Date'); + expect(errorMessage).toContain('Post 2 without Date'); + expect(errorMessage).toContain('/blog/post3-no-date-no-title'); // Verify slug is used when title is null + }
237-244: LGTM: Good error handling coverageThe test properly verifies that non-Error exceptions (like strings) are wrapped in CustomError. The mock cleanup is correctly handled.
Optionally, consider adding a comment to clarify the intentional use of a string error:
it('should wrap non-Error exceptions in CustomError', async () => { jest.doMock('../config/posts.json', () => mockRssData, { virtual: true }); + // Intentionally reject with a non-Error value to test error wrapping const mockWriteFile = jest.spyOn(fs.promises, 'writeFile').mockRejectedValue('String error');tests/markdown/check-edit-links.test.ts (3)
209-216: Consider strengthening the AbortController test.This test immediately aborts the controller and mocks
fetchto reject with 'Aborted', but it doesn't verify that the abort signal is actually passed tofetchor thatfetchrespects the signal. The test would pass even ifprocessBatchdoesn't use the AbortController at all.To properly validate abort behavior, consider:
- Verifying that
mockFetchis called with the abort signal:expect(mockFetch).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ signal: expect.any(AbortSignal) }))- Or testing a real abort scenario where fetch is in-flight when abort is called
265-273: Consider using jest.spyOn on the imported module instead of dynamic require.The dynamic
requirewithglobal-requiredisable is a code smell. SincegeneratePathsis already imported at line 10, you could usejest.spyOnon the imported module:- // eslint-disable-next-line global-require - const mockGeneratePaths = jest.spyOn(require('../../scripts/markdown/check-edit-links'), 'generatePaths'); + const checkEditLinks = await import('../../scripts/markdown/check-edit-links'); + const mockGeneratePaths = jest.spyOn(checkEditLinks, 'generatePaths');However, if module mocking constraints require the current approach, the existing implementation is acceptable.
191-200: Note: Testing implementation detail rather than behavior.This test verifies that
clearTimeoutis called, which is an implementation detail rather than observable behavior. While this helps ensure proper cleanup, it can make tests brittle if the implementation changes to use a different cleanup mechanism.Consider whether this test adds significant value, or if resource cleanup could be validated through other means (e.g., integration tests that detect leaked timers).
npm/runners/build-meetings-runner.ts (1)
43-54: Consider adding a direct-execution guard, not just a test-env guardRight now the CLI auto-runs whenever
NODE_ENV !== 'test'andVITEST_WORKER_IDis unset, even if this module is imported from other runtime code. To avoid side effects on import and stay consistent with other runners, consider also guarding on the module being the entry script (e.g., via afileURLToPath(import.meta.url) === process.argv[1]check) so only direct CLI execution runs the IIFE.
Based on learningsnpm/runners/build-post-list-runner.ts (1)
11-51: Tighten types and error context for better diagnosticsTwo small improvements here:
postDirectoriesis currently typed asstring[][], but it’s always[directoryPath, routePrefix]. A tuple type like[string, string][]would better express intent and catch misuses at compile time.- In the error detail you log
options.outputPath, which is oftenundefinedwhen relying on the default. Logging the computedoutputPath(and optionallybasePath) would make runner failures easier to diagnose.For example:
- const customError = CustomError.fromError(error, { - category: 'script', - operation: 'runBuildPostList', - detail: `Build post list failed with output path: ${options.outputPath}` - }); + const customError = CustomError.fromError(error, { + category: 'script', + operation: 'runBuildPostList', + detail: `Build post list failed with output path: ${outputPath}` + });Based on learnings
tests/integration/compose-blog-runner.int.test.ts (2)
493-515: Make the CustomError context test fail if no error is thrownIn
wraps errors in CustomError with proper context, ifrunComposeBlogever stops throwing for this input, the test will pass without executing any assertions. To keep it robust, explicitly assert that an error is thrown:it('wraps errors in CustomError with proper context', async () => { expect.assertions(3); const answers = { /* ... */ }; try { await runComposeBlog({ answers, outputPath: join(outputDir, 'error-test.md') }); // Optional: explicitly fail if no error // fail('Expected runComposeBlog to throw'); } catch (error) { expect(error).toBeInstanceOf(CustomError); if (error instanceof CustomError) { expect(error.context.operation).toBe('runComposeBlog'); expect(error.context.category).toBe('script'); } } });This guarantees the test fails if the expected error is not produced.
671-707: Default-path test is correct but touches real project filesThe
returns default path when outputPath is not providedtest accurately verifies the default behavior by callingrunComposeBlogwithoutoutputPathand checking formarkdown/blog/<slug>.md. It mitigates conflicts via a unique title and attempts cleanup.If this ever becomes noisy (e.g., failures leaving behind files), consider routing the default path via a configurable base directory in
runComposeBlogso tests can keep everything underos.tmpdir(). For now, the current approach is acceptable.tests/integration/helpers/build-pages-lock.ts (1)
37-109: Polling logic forwaitForPagesBuildis robust; only minor optional refactorsThe timeout-based polling of the
pagesdirectory, checking forblog/docs/aboutand.mdxfiles, is a reasonable readiness check for these integration tests. Swallowing IO errors and always sleeping between iterations keeps the helper resilient without over-complicating it. If you want, you could factor the['blog', 'docs', 'about']list into a shared constant or log which subdir satisfied the condition, but that’s purely optional.tests/integration/helpers/file-utils.ts (1)
4-28: Recursive file walker is correct; consider normalizing directory separatorsThe recursive
getAllFilesimplementation is straightforward and correct for test usage. If you ever need these directory entries to be reused as OS paths (not just for assertions), you might consider usingpath.sepinstead of a hard-coded/when pushing directory names:- if (stat.isDirectory()) { - fileList.push(`${filePath}/`); + if (stat.isDirectory()) { + fileList.push(`${filePath}${path.sep}`);But as a test-only helper, the current approach is acceptable.
scripts/adopters/index.ts (1)
15-50: Robust validation & context wrapping; consider aligning context keys with runner usageThe input validation and contextual wrapping around
writeJSONlook solid and give good diagnostics. One small polish: the early errors attachcontext.function = 'buildAdoptersList'while the catch block (and the runner) work withcontext.operation. Usingoperationconsistently (or copyingfunctionintooperationbefore rethrow) would make downstream error consumers simpler and more uniform.tests/integration/build-meetings-runner.int.test.ts (1)
195-201: Make export/type-safety test async and reuse the mockAs written,
runBuildMeetings()is invoked in a synchronous test and the returned Promise is never awaited. IfrunBuildMeetingsrejects (e.g., becausemockCalendarisn’t configured for this specific call), Jest may see an unhandled rejection instead of a normal assertion failure, and the test doesn’t explicitly verify successful completion.You can keep the “returns a Promise” check but make the flow explicit:
- it('should export the runBuildMeetings function', () => { - expect(typeof runBuildMeetings).toBe('function'); - // Verify it's a Promise-returning function - expect(runBuildMeetings()).toBeInstanceOf(Promise); - }); + it('should export the runBuildMeetings function', async () => { + expect(typeof runBuildMeetings).toBe('function'); + mockCalendar.mockResolvedValue({ data: { items: mockEvents } }); + + const promise = runBuildMeetings({ outputPath }); + expect(promise).toBeInstanceOf(Promise); + await expect(promise).resolves.toBeUndefined(); + });This matches the integration style used elsewhere and ensures the async path is properly exercised.
tests/integration/build-tools.int.test.ts (1)
227-287: Comprehensive error-path coverage and correct Promise assertionsThe error-handling scenarios around GitHub failures, empty responses, and file-write issues are well covered here, and it’s good to see the Promise assertions using
.resolves.toBeUndefined()/.rejects.toThrow()instead of.resolves.not.toThrow(). The CustomError context checks (category, operation, detail) in the surrounding tests also align nicely with the runner’s contract.npm/runners/build-adopters-list-runner.ts (1)
46-64: Consider avoiding double logging between runner and CLI wrapperBoth the inner
runBuildAdoptersListcatch and the CLI IIFE log an error vialogger.errorfor the same failure before rethrowing / exiting. That’s functionally safe, but in CLI usage it can result in duplicated log lines for a single error.If you want to align more closely with the “runners define error boundaries, top-level orchestrator/CLI does the logging” pattern, you could either:
- Drop the
logger.errorcall insiderunBuildAdoptersListand let the CLI (or npm/index.ts) be responsible for logging, or- Keep the runner log and simplify the CLI wrapper to just
process.exit(1)after logging once.Not mandatory, but it would make logs a bit cleaner.
tests/integration/build-finance-info-list-runner.int.test.ts (1)
1-611: Finance info runner integration tests are thorough and well-isolatedThe suite exercises creation, year selection, default/custom path handling, error paths (CustomError cases), and several edge cases using per-test temp directories, which is solid. If you ever find this file getting unwieldy, consider small helpers for repeated “write Expenses/ExpensesLink YAML and run” sequences, but that’s optional.
tests/integration/build-pages-runner.int.test.ts (2)
402-407: Avoid unawaitedrunBuildPagescall in “Function Export” testThe export test calls
runBuildPages()without awaiting or returning the Promise, which can leave work running in the background and potentially overlap with other tests.Given other tests already prove the function runs end-to-end, this check can safely be simplified to a pure export assertion:
- describe('Function Export', () => { - it('should export the runner function', () => { - expect(typeof runBuildPages).toBe('function'); - // Verify it's a Promise-returning function - expect(runBuildPages()).toBeInstanceOf(Promise); - }); - }); + describe('Function Export', () => { + it('should export the runner function', () => { + expect(typeof runBuildPages).toBe('function'); + }); + });This still validates the public API without introducing extra concurrent runs.
14-90: Shared tempDir/targetDir across tests is acceptable but could be isolated furtherUsing
beforeAllto seed a single tempDir/sourceDir/targetDir and repeatedly invokingrunBuildPagesworks here (and you explicitly test idempotency), but it does mean tests share filesystem state and may mask ordering-sensitive issues.If you ever see flakiness, consider moving targetDir setup/cleanup to
beforeEach/afterEachfor stronger isolation, while keeping the current structure as a starting point.tests/integration/fixtures/dashboard-fixtures.ts (1)
1-500: Dashboard fixtures align well with expected GraphQL shapesThe mock structures (issues, PRs, pagination, rateLimit, errors) look consistent with the runner’s needs and give good coverage for dashboard scenarios. If any future tests start comparing timestamps directly, consider switching the
new Date(...).toISOString()values to fixed ISO strings to make them fully deterministic, but it isn’t necessary with the current usage. Based on learnings.tests/integration/build-post-list.int.test.ts (3)
183-198: Consolidate the duplicate toc-structure checksThe tests “toc entries in docs have expected fields” and “docs items with toc have valid structure” effectively assert the same shape (
content,slug,lvl,iwith expected types) onitem.toc.You could keep just one of these (or merge them) to reduce duplication and keep future maintenance simpler, without losing coverage.
Also applies to: 232-243
271-282: Clarify what is being asserted for directories without_section.mdxThe test name and comment refer to “directories without
_section.mdx”, but the filter condition is effectively “all section items whose title does not include ‘Welcome’”. That will also include sections that do have_section.mdxfiles.Either tightening the filter (e.g., based on
isSectionplus some flag or slug heuristic), or relaxing the description (e.g., “section items have a string title”) would make the test intent clearer and avoid future confusion.
351-373: Tighten error-handling tests by asserting error messagesThe invalid
basePathandwriteFilePathtests assert specific error messages, but thepostDirectoriescase only checks that some error is thrown. Given the runners are expected to surface clear, descriptive errors, it would be helpful to also assert on the message (even partially) here.For example:
- await expect( - runBuildPostList({ - outputPath: resolve(tempDir, 'empty-dirs-test.json'), - basePath: 'pages', - postDirectories: [] - }) - ).rejects.toThrow(); + await expect( + runBuildPostList({ + outputPath: resolve(tempDir, 'empty-dirs-test.json'), + basePath: 'pages', + postDirectories: [] + }) + ).rejects.toThrow('postDirectories');This aligns with the actual error thrown by
buildPostList("postDirectories array is empty") and maintains consistency with the adjacent validation tests.tests/integration/fixtures/nock-helpers.ts (2)
9-11: Update documentation comments to match the new.int.test.tsfilenamesThe header comments still reference
tests/integration/build-dashboard.test.tsandtests/integration/build-tools.test.ts, but the integration suites are now named with.int.test.ts.Consider updating these comments to avoid confusion:
-// Tests: tests/integration/build-dashboard.test.ts +// Tests: tests/integration/build-dashboard.int.test.ts ... -// Tests: tests/integration/build-tools.test.ts +// Tests: tests/integration/build-tools.int.test.tsAlso applies to: 205-207
69-121: Extract shared GraphQL body parsing logic to reduce duplicationBoth
setupDashboardPaginationMocksandsetupPullRequestMocksimplement identical body parsing logic (lines 77–93 and 135–151). The parsing handlesBuffer, string, and object cases identically, differing only in the fallback value.Consider extracting a helper to centralize this logic:
function parseGraphQLRequestBody(path: string, body: string | Buffer | any, fallback: any) { try { if (body instanceof Buffer) { return JSON.parse(body.toString()); } if (typeof body === 'string') { if (body === '/graphql' || body === path) { return fallback; } return JSON.parse(body); } return body; } catch { return fallback; } }Then call it in both callbacks with the appropriate fallback (
paginationMocks.firstPageoremptyResponse). This keeps routing logic clear while centralizing the parsing behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (38)
config/all-tags.json(1 hunks)config/meetings.json(1 hunks)config/newsroom_videos.json(1 hunks)jest.config.js(1 hunks)markdown/blog/default-path-test-1763304991853.md(1 hunks)markdown/blog/default-path-test.md(1 hunks)npm/runners/build-adopters-list-runner.ts(2 hunks)npm/runners/build-dashboard-runner.ts(3 hunks)npm/runners/build-meetings-runner.ts(2 hunks)npm/runners/build-newsroom-videos-runner.ts(2 hunks)npm/runners/build-pages-runner.ts(2 hunks)npm/runners/build-post-list-runner.ts(3 hunks)npm/runners/build-tools-runner.ts(1 hunks)npm/runners/case-studies-runner.ts(1 hunks)package.json(2 hunks)scripts/adopters/index.ts(1 hunks)scripts/index.ts(1 hunks)tests/adopters/index.test.ts(1 hunks)tests/build-meetings.test.ts(2 hunks)tests/build-rss.test.ts(4 hunks)tests/check-locales.test.ts(1 hunks)tests/index.test.ts(1 hunks)tests/integration/build-adopters-list-runner.int.test.ts(1 hunks)tests/integration/build-dashboard.int.test.ts(1 hunks)tests/integration/build-finance-info-list-runner.int.test.ts(1 hunks)tests/integration/build-meetings-runner.int.test.ts(1 hunks)tests/integration/build-newsroom-videos-runner.int.test.ts(1 hunks)tests/integration/build-pages-runner.int.test.ts(1 hunks)tests/integration/build-post-list.int.test.ts(1 hunks)tests/integration/build-tools.int.test.ts(1 hunks)tests/integration/case-studies-runner.int.test.ts(1 hunks)tests/integration/compose-blog-runner.int.test.ts(1 hunks)tests/integration/fixtures/dashboard-fixtures.ts(1 hunks)tests/integration/fixtures/nock-helpers.ts(1 hunks)tests/integration/fixtures/tools-fixtures.ts(1 hunks)tests/integration/helpers/build-pages-lock.ts(1 hunks)tests/integration/helpers/file-utils.ts(1 hunks)tests/markdown/check-edit-links.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (31)
📓 Common learnings
Learnt from: sagarkori143
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-06-20T14:47:22.389Z
Learning: Runner files in the npm/runners directory should follow consistent error handling patterns, throwing new Error instances with descriptive messages rather than just rethrowing caught errors.
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 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:
tests/integration/build-adopters-list-runner.int.test.tstests/build-meetings.test.tsnpm/runners/build-pages-runner.tstests/adopters/index.test.tstests/integration/build-newsroom-videos-runner.int.test.tsjest.config.jstests/integration/case-studies-runner.int.test.tstests/integration/build-post-list.int.test.ts
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 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:
tests/integration/build-adopters-list-runner.int.test.tstests/build-meetings.test.tsnpm/runners/build-pages-runner.tstests/adopters/index.test.tstests/build-rss.test.tstests/integration/build-newsroom-videos-runner.int.test.tstests/integration/compose-blog-runner.int.test.tstests/integration/helpers/build-pages-lock.tstests/integration/helpers/file-utils.tsjest.config.jstests/integration/case-studies-runner.int.test.tstests/integration/build-pages-runner.int.test.tspackage.jsontests/integration/build-post-list.int.test.ts
📚 Learning: 2024-11-10T18:16:22.273Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 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-adopters-list-runner.int.test.tstests/index.test.tstests/build-meetings.test.tsnpm/runners/build-post-list-runner.tstests/markdown/check-edit-links.test.tsnpm/runners/build-pages-runner.tsscripts/adopters/index.tstests/adopters/index.test.tstests/integration/build-finance-info-list-runner.int.test.tstests/integration/build-newsroom-videos-runner.int.test.tstests/integration/compose-blog-runner.int.test.tsnpm/runners/build-adopters-list-runner.tstests/integration/helpers/build-pages-lock.tsscripts/index.tstests/integration/build-tools.int.test.tstests/integration/case-studies-runner.int.test.tstests/integration/build-pages-runner.int.test.tstests/integration/build-meetings-runner.int.test.tstests/integration/build-post-list.int.test.ts
📚 Learning: 2024-11-29T19:42:31.175Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3265
File: scripts/tools/tools-object.js:93-98
Timestamp: 2024-11-29T19:42:31.175Z
Learning: In `scripts/tools/tools-object.js`, when validation fails in the `convertTools` function, errors should be thrown or rejected instead of only logging them, to ensure proper error propagation and handling.
Applied to files:
tests/integration/build-adopters-list-runner.int.test.tstests/check-locales.test.tstests/integration/build-tools.int.test.tstests/integration/case-studies-runner.int.test.tstests/integration/build-meetings-runner.int.test.ts
📚 Learning: 2025-06-20T14:47:22.389Z
Learnt from: sagarkori143
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-06-20T14:47:22.389Z
Learning: Runner files in the npm/runners directory should follow consistent error handling patterns, throwing new Error instances with descriptive messages rather than just rethrowing caught errors.
Applied to files:
tests/integration/build-adopters-list-runner.int.test.tstests/markdown/check-edit-links.test.tstests/build-rss.test.tstests/integration/build-newsroom-videos-runner.int.test.tsnpm/runners/build-adopters-list-runner.tstests/integration/build-tools.int.test.tstests/integration/case-studies-runner.int.test.tstests/integration/build-pages-runner.int.test.tstests/integration/build-meetings-runner.int.test.tstests/integration/build-post-list.int.test.ts
📚 Learning: 2024-11-01T13:32:15.472Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3101
File: tests/fixtures/rssData.js:1-57
Timestamp: 2024-11-01T13:32:15.472Z
Learning: In the `tests/fixtures/rssData.js` file of the `asyncapi/website` project, tests for edge cases such as empty strings for title or excerpt, very long text content, international characters (UTF-8), or malformed URLs in `slug` or `cover` are not necessary because these cases will not occur.
Applied to files:
tests/integration/build-adopters-list-runner.int.test.tstests/markdown/check-edit-links.test.tstests/integration/build-finance-info-list-runner.int.test.tstests/integration/fixtures/tools-fixtures.tstests/build-rss.test.tstests/integration/build-newsroom-videos-runner.int.test.tstests/integration/compose-blog-runner.int.test.tstests/integration/fixtures/dashboard-fixtures.tsmarkdown/blog/default-path-test.mdtests/integration/build-tools.int.test.tstests/integration/case-studies-runner.int.test.tstests/integration/build-pages-runner.int.test.tstests/integration/fixtures/nock-helpers.tstests/integration/build-dashboard.int.test.tsmarkdown/blog/default-path-test-1763304991853.mdtests/integration/build-post-list.int.test.ts
📚 Learning: 2024-11-29T17:36:09.783Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3378
File: tests/markdown/check-markdown.test.js:133-138
Timestamp: 2024-11-29T17:36:09.783Z
Learning: When testing the 'main' function in 'check-markdown.test.js', it's acceptable to simply ensure it doesn't throw any errors, as the functions it calls are already tested elsewhere.
Applied to files:
tests/integration/build-adopters-list-runner.int.test.tstests/index.test.tstests/markdown/check-edit-links.test.tstests/adopters/index.test.tstests/build-rss.test.tstests/integration/build-newsroom-videos-runner.int.test.tstests/check-locales.test.tstests/integration/compose-blog-runner.int.test.tstests/integration/build-tools.int.test.tstests/integration/case-studies-runner.int.test.tstests/integration/build-pages-runner.int.test.tstests/integration/build-dashboard.int.test.tstests/integration/build-meetings-runner.int.test.tstests/integration/build-post-list.int.test.ts
📚 Learning: 2025-06-19T13:51:27.459Z
Learnt from: sagarkori143
Repo: asyncapi/website PR: 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:
tests/integration/build-adopters-list-runner.int.test.tsnpm/runners/build-post-list-runner.tstests/integration/build-newsroom-videos-runner.int.test.tsnpm/runners/build-adopters-list-runner.tstests/integration/build-tools.int.test.tstests/integration/case-studies-runner.int.test.tstests/integration/build-pages-runner.int.test.tsnpm/runners/build-dashboard-runner.tstests/integration/build-dashboard.int.test.tstests/integration/build-meetings-runner.int.test.ts
📚 Learning: 2025-01-18T08:44:43.614Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-18T08:44:43.614Z
Learning: In JavaScript test files within the AsyncAPI website project, TypeScript file imports must include the .ts extension to avoid lint errors, even though the files being imported are JavaScript files.
Applied to files:
tests/build-meetings.test.tsjest.config.js
📚 Learning: 2024-11-01T09:55:20.531Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 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.
Applied to files:
tests/build-meetings.test.tstests/markdown/check-edit-links.test.tstests/adopters/index.test.tstests/build-rss.test.tstests/integration/build-newsroom-videos-runner.int.test.tstests/integration/compose-blog-runner.int.test.tstests/integration/build-tools.int.test.tsjest.config.jstests/integration/build-pages-runner.int.test.tstests/integration/fixtures/nock-helpers.tstests/integration/build-dashboard.int.test.tstests/integration/build-meetings-runner.int.test.ts
📚 Learning: 2024-10-19T15:26:43.683Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3301
File: scripts/markdown/check-markdown.js:127-128
Timestamp: 2024-10-19T15:26:43.683Z
Learning: In `scripts/markdown/check-markdown.js`, synchronous file reading with `fs.readFileSync` is necessary because the files need to be processed synchronously.
Applied to files:
tests/build-meetings.test.ts
📚 Learning: 2024-10-18T17:28:41.718Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-10-18T17:28:41.718Z
Learning: In `scripts/markdown/check-markdown.js`, when processing large markdown files with recursion and loops, synchronous file reads (`fs.readFileSync`) are preferred over asynchronous reads.
Applied to files:
tests/build-meetings.test.tstests/markdown/check-edit-links.test.ts
📚 Learning: 2024-11-01T09:35:23.912Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 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.tstests/build-rss.test.tsscripts/index.tstests/integration/build-meetings-runner.int.test.tstests/integration/build-post-list.int.test.ts
📚 Learning: 2024-11-10T18:16:35.551Z
Learnt from: vishvamsinh28
Repo: asyncapi/website PR: 3284
File: scripts/build-post-list.js:28-46
Timestamp: 2024-11-10T18:16:35.551Z
Learning: In the `scripts/build-post-list.js` JavaScript file, tests rely on the `result` object being a global variable. Moving it inside the `buildPostList` function causes tests to fail; therefore, keep `result` as a global variable.
Applied to files:
npm/runners/build-post-list-runner.tstests/integration/compose-blog-runner.int.test.tstests/integration/build-post-list.int.test.ts
📚 Learning: 2025-06-20T14:47:22.389Z
Learnt from: sagarkori143
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-06-20T14:47:22.389Z
Learning: The AsyncAPI website modularization PR uses Promise.allSettled for parallel execution of build tasks, which provides better performance and fault isolation compared to sequential execution.
Applied to files:
npm/runners/build-post-list-runner.tstests/integration/build-newsroom-videos-runner.int.test.tstests/integration/helpers/build-pages-lock.tsscripts/index.tstests/integration/build-meetings-runner.int.test.ts
📚 Learning: 2025-06-19T13:49:29.796Z
Learnt from: sagarkori143
Repo: asyncapi/website PR: 4192
File: npm/runners/build-newsroom-videos-runner.ts:8-15
Timestamp: 2025-06-19T13:49:29.796Z
Learning: In the AsyncAPI website modularization project, error handling is centralized in the top-level orchestrator function (npm/index.ts) with comprehensive logging and context. Individual runner functions in npm/runners/ are kept simple and let errors propagate naturally to the centralized handler, avoiding redundant try-catch blocks that only rethrow errors.
Applied to files:
npm/runners/case-studies-runner.tsnpm/runners/build-adopters-list-runner.tstests/integration/build-tools.int.test.tstests/integration/case-studies-runner.int.test.tstests/integration/build-meetings-runner.int.test.ts
📚 Learning: 2025-01-19T04:51:41.255Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3557
File: tests/fixtures/markdown/check-edit-links-data.js:3-11
Timestamp: 2025-01-19T04:51:41.255Z
Learning: In the AsyncAPI website repository, the test data in `tests/fixtures/markdown/check-edit-links-data.js` intentionally includes inconsistent paths (with and without 'docs' prefix) to verify the script's ability to normalize and handle ambiguous path structures.
Applied to files:
tests/markdown/check-edit-links.test.tstests/adopters/index.test.tstests/integration/fixtures/tools-fixtures.tstests/build-rss.test.tstests/integration/compose-blog-runner.int.test.tstests/integration/fixtures/dashboard-fixtures.tsmarkdown/blog/default-path-test.mdtests/integration/build-tools.int.test.tstests/integration/build-pages-runner.int.test.tstests/integration/build-dashboard.int.test.tsmarkdown/blog/default-path-test-1763304991853.mdtests/integration/build-post-list.int.test.ts
📚 Learning: 2025-01-14T15:12:29.840Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3557
File: scripts/markdown/check-edit-links.js:83-88
Timestamp: 2025-01-14T15:12:29.840Z
Learning: In the AsyncAPI website's edit link generation logic, the `editUrls` configuration (config/edit-page-config.json) includes a fallback entry with an empty value that acts as a catch-all matcher. Therefore, `editOptions.find()` will always return a match, making null checks on the result unnecessary and untestable.
Applied to files:
tests/markdown/check-edit-links.test.ts
📚 Learning: 2025-01-08T15:15:00.759Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3557
File: scripts/markdown/check-editlinks.js:58-59
Timestamp: 2025-01-08T15:15:00.759Z
Learning: In the AsyncAPI codebase, batch processing operations (like in the Dashboard script and check-editlinks.js) follow a sequential pattern using await in loops, which is the preferred approach for maintaining consistency across the codebase.
Applied to files:
tests/markdown/check-edit-links.test.ts
📚 Learning: 2025-04-20T16:05:16.482Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3950
File: scripts/utils/check-locales.ts:122-129
Timestamp: 2025-04-20T16:05:16.482Z
Learning: In the AsyncAPI website project, Next.js throws errors at runtime when locale files are missing, making additional validation for missing files unnecessary in the check-locales script.
Applied to files:
tests/markdown/check-edit-links.test.tstests/check-locales.test.tstests/integration/build-pages-runner.int.test.tstests/integration/build-post-list.int.test.ts
📚 Learning: 2024-10-18T17:24:45.053Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3301
File: scripts/markdown/check-markdown.js:0-0
Timestamp: 2024-10-18T17:24:45.053Z
Learning: In `scripts/markdown/check-markdown.js`, the script is not run in an asynchronous environment, so refactoring it to use `async/await` is unnecessary.
Applied to files:
tests/markdown/check-edit-links.test.tsnpm/runners/build-pages-runner.ts
📚 Learning: 2025-02-16T12:57:24.575Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-16T12:57:24.575Z
Learning: Tests failing after dependency upgrades indicate potential breaking changes and might require code changes rather than test modifications to support newer versions.
Applied to files:
tests/adopters/index.test.tstests/integration/compose-blog-runner.int.test.tstests/integration/build-tools.int.test.ts
📚 Learning: 2024-12-02T05:52:49.547Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3265
File: tests/helper/toolsObjectData.js:1-18
Timestamp: 2024-12-02T05:52:49.547Z
Learning: In `tests/helper/toolsObjectData.js`, using the hard-coded repository ID directly in the URL is acceptable when creating mock data for tests, as we are not asserting anything at `REPO_ID`.
Applied to files:
tests/integration/fixtures/tools-fixtures.tstests/build-rss.test.tstests/integration/fixtures/dashboard-fixtures.tstests/integration/build-tools.int.test.ts
📚 Learning: 2025-03-23T12:42:49.503Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3950
File: scripts/utils/check-locales.ts:138-143
Timestamp: 2025-03-23T12:42:49.503Z
Learning: For the locale validation function in check-locales.ts, the team has decided that modifying the outer scope variable 'hasErrors' inside the forEach loop is acceptable despite the ESLint warning about unsafe references in loop functions.
Applied to files:
tests/check-locales.test.ts
📚 Learning: 2024-11-01T11:40:51.617Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3265
File: scripts/tools/tools-object.js:49-51
Timestamp: 2024-11-01T11:40:51.617Z
Learning: In JavaScript code, prefer using a custom LogService with Winston for logging errors instead of `console.error` or ad-hoc logging.
Applied to files:
npm/runners/build-adopters-list-runner.ts
📚 Learning: 2025-01-03T08:14:02.138Z
Learnt from: JeelRajodiya
Repo: asyncapi/website PR: 3423
File: tests/index.test.js:2-7
Timestamp: 2025-01-03T08:14:02.138Z
Learning: The user (JeelRajodiya) has stated a preference to keep the test files in CommonJS rather than migrating them to TypeScript.
Applied to files:
jest.config.js
📚 Learning: 2024-12-30T11:00:42.064Z
Learnt from: JeelRajodiya
Repo: asyncapi/website PR: 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.jspackage.jsontests/integration/build-post-list.int.test.ts
📚 Learning: 2025-01-08T15:16:27.655Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3557
File: .github/workflows/check-edit-links.yml:25-29
Timestamp: 2025-01-08T15:16:27.655Z
Learning: In GitHub workflows running scripts with process.exit statements for error handling (like check-editlinks.js in asyncapi/website), avoid adding error suppression (|| true) at the workflow level as it would mask the intended error reporting mechanism.
Applied to files:
npm/runners/build-dashboard-runner.ts
📚 Learning: 2024-11-25T18:41:29.632Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3378
File: scripts/markdown/check-markdown.js:1-1
Timestamp: 2024-11-25T18:41:29.632Z
Learning: When updating workflows for the AsyncAPI website repository, use `.github/workflows/if-nodejs-pr-testing.yml` to include environment variables and secrets for Node.js PR testing.
Applied to files:
npm/runners/build-dashboard-runner.ts
📚 Learning: 2024-10-18T17:37:50.602Z
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3301
File: scripts/markdown/check-markdown.js:31-31
Timestamp: 2024-10-18T17:37:50.602Z
Learning: In `scripts/markdown/check-markdown.js`, the `frontmatter` object cannot have a property named `hasOwnProperty`, so using `frontmatter.hasOwnProperty(attr)` is acceptable.
Applied to files:
tests/integration/build-post-list.int.test.ts
🪛 markdownlint-cli2 (0.18.1)
markdown/blog/default-path-test.md
21-21: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
27-27: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
33-33: Bare URL used
(MD034, no-bare-urls)
45-45: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
45-45: Bare URL used
(MD034, no-bare-urls)
46-46: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
47-47: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
49-49: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
62-62: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
68-68: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
markdown/blog/default-path-test-1763304991853.md
21-21: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
27-27: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
33-33: Bare URL used
(MD034, no-bare-urls)
45-45: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
45-45: Bare URL used
(MD034, no-bare-urls)
46-46: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
47-47: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
49-49: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
62-62: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
68-68: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
3 participants
Description
Summary by CodeRabbit
New Features
Content
Refactor
Tests
Chores