feat: enable tools view regeneration at build time #4511
Conversation
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds two functions to scripts/build-tools.ts — Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Workflow as Start Workflow
participant Index as scripts/index.ts
participant BuildTools as scripts/build-tools.ts
participant Combine as combineTools()
Workflow->>Index: start()
Index->>BuildTools: buildToolsManual(automatedPath, manualPath, toolsPath, tagsPath)
BuildTools->>BuildTools: verify input files exist
alt inputs present
BuildTools->>BuildTools: read automated & manual data
BuildTools->>Combine: combineAutomatedAndManualTools(automated, manual, toolsPath, tagsPath)
Combine->>Combine: run combineTools + capture/log errors
alt combine success
BuildTools->>BuildTools: write tools and tags outputs
BuildTools-->>Index: success
else combine error
BuildTools-->>Index: propagate error
end
else missing input
BuildTools-->>Index: error (missing file)
end
Index->>Workflow: continue
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@asyncapi/bounty_team |
princerajpoot20
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg and
sambhavgupta0705
as code owners
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4511--asyncapi-website.netlify.app/ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4511 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 780 799 +19
Branches 144 146 +2
=========================================
+ Hits 780 799 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/build-tools.ts (1)
62-65: Enhance JSDoc to clarify usage.The JSDoc could be more detailed to explain when to use this function versus
buildTools, and that it requires a pre-existing automated tools file.Consider expanding the documentation:
/** - * Builds tools manually by combining existing automated tools with manual tools. - * This function is used to ensure that it reflects changes in tools-manual.json. + * Builds tools by combining pre-existing automated tools with manual tools. + * + * This function is designed for regular builds where the automated tools file + * has already been generated by the GitHub workflow. Unlike buildTools, this + * does NOT fetch data from the GitHub API, making it suitable for frequent + * builds without exhausting API rate limits. + * + * @param automatedToolsPath - Path to the pre-existing automated tools JSON file + * @param manualToolsPath - Path to the manual tools JSON file + * @param toolsPath - Output path for the combined tools JSON file + * @param tagsPath - Output path for the tags JSON file + * @throws {Error} If required files are missing or combination fails */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/build-tools.ts(3 hunks)scripts/index.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: asyncapi-bot
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch
📚 Learning: 2024-11-01T12:49:32.805Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3136
File: scripts/tools/combine-tools.js:122-125
Timestamp: 2024-11-01T12:49:32.805Z
Learning: In `scripts/tools/combine-tools.js`, the existing URL parsing logic for `repoUrl` without additional error handling is acceptable and does not require changes.
Applied to files:
scripts/build-tools.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:
scripts/index.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:
scripts/index.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:
scripts/index.ts
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test NodeJS PR - macos-13
- GitHub Check: Test NodeJS PR - windows-latest
🔇 Additional comments (6)
scripts/index.ts (3)
10-10: LGTM!The import is correct and the function is properly utilized in the workflow.
14-17: LGTM!The path constants are correctly defined and will be properly resolved relative to the script directory.
43-46: No issues found — code is correct.The verification confirms that
tools-automated.jsonexists in the repository (35706 bytes) and is tracked by git. ThebuildToolsManualfunction properly handles file reads with try-catch error handling, logging errors and re-throwing with descriptive messages. The implementation is sound and the file dependency is satisfied.scripts/build-tools.ts (3)
12-32: LGTM!The wrapper function provides consistent error handling and logging for tool combination operations. The approach of logging before re-throwing maintains proper error propagation while aiding debugging.
56-56: LGTM!Using the wrapper function here ensures consistent error handling across both build paths (full rebuild vs. manual-only rebuild).
95-95: LGTM!The export statement correctly exposes both build functions for use in other modules.
|
Update: The implementation part is done. Will work on the coverage part next. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/build-tools.test.ts (1)
138-146: Remove redundant file write operation.Line 141 writes to
automatedToolsPath, but this file is already created and populated in thebeforeAllhook (line 43). This write operation is unnecessary for testing the missing manual file scenario.Apply this diff to simplify the test:
it('should handle missing manual tools file error', async () => { const invalidManualPath = resolve(testDir, 'nonexistent-manual.json'); - fs.writeFileSync(automatedToolsPath, JSON.stringify(mockConvertedData)); - await expect( buildToolsManual(automatedToolsPath, invalidManualPath, toolsPath, tagsPath) ).rejects.toThrow('Manual tools file not found'); });scripts/build-tools.ts (1)
62-89: Enhance JSDoc for consistency withbuildToolsdocumentation.The JSDoc for
buildToolsManualis minimal compared tobuildTools(lines 34-47). For consistency and better maintainability, consider adding@paramand@throwstags.Apply this diff to enhance the documentation:
/** * Builds tools manually by combining existing automated tools with manual tools. * This function is used to ensure that it reflects changes in tools-manual.json. + * + * @param automatedToolsPath - The file path from which the automated tools data is read. + * @param manualToolsPath - The file path from which the manual tools data is read. + * @param toolsPath - The file path where the combined tools data will be written. + * @param tagsPath - The file path where the tags data will be written. + * @throws {Error} If the automated or manual tools files are not found, or if an error occurs during the build process. */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/build-tools.ts(3 hunks)tests/build-tools.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: asyncapi-bot
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch
Learnt from: akshatnema
Repo: asyncapi/website PR: 3136
File: scripts/tools/combine-tools.js:122-125
Timestamp: 2024-11-01T12:49:32.805Z
Learning: In `scripts/tools/combine-tools.js`, the existing URL parsing logic for `repoUrl` without additional error handling is acceptable and does not require changes.
📚 Learning: 2024-11-01T12:49:32.805Z
Learnt from: akshatnema
Repo: asyncapi/website PR: 3136
File: scripts/tools/combine-tools.js:122-125
Timestamp: 2024-11-01T12:49:32.805Z
Learning: In `scripts/tools/combine-tools.js`, the existing URL parsing logic for `repoUrl` without additional error handling is acceptable and does not require changes.
Applied to files:
scripts/build-tools.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:
scripts/build-tools.tstests/build-tools.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:
scripts/build-tools.ts
📚 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-tools.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/build-tools.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-tools.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/build-tools.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-tools.test.ts
🧬 Code graph analysis (1)
scripts/build-tools.ts (3)
tests/fixtures/buildToolsData.ts (1)
manualTools(79-79)scripts/tools/combine-tools.ts (1)
combineTools(230-230)scripts/helpers/logger.ts (1)
logger(18-18)
🔇 Additional comments (9)
tests/build-tools.test.ts (5)
6-6: LGTM!The import correctly adds
buildToolsManualto support the new test cases below.
94-104: LGTM!This test validates the happy path for
buildToolsManual, ensuring it correctly combines existing automated and manual tool files into the expected output structure.
106-120: LGTM!This test confirms that errors from
combineToolsare properly caught, wrapped with context, and propagated through the newcombineAutomatedAndManualToolswrapper.
122-128: LGTM!This test validates that
buildToolsManualproperly handles invalid directory paths and provides a clear error message.
130-136: LGTM!This test confirms the file existence check for
automatedToolsPathworks correctly and throws a descriptive error when the file is missing.scripts/build-tools.ts (4)
12-32: LGTM!The
combineAutomatedAndManualToolswrapper provides consistent error handling and logging for the combine operation. The JSDoc is clear and the implementation properly wraps errors with context.
56-56: LGTM!Good refactor to use the new
combineAutomatedAndManualToolswrapper for consistency.
73-84: LGTM!The file existence checks provide clear error messages when required input files are missing, addressing the feedback from previous reviews. The implementation correctly reads and combines the tool data.
104-104: LGTM!The export correctly exposes
buildToolsManualalongsidebuildToolsfor use in the build pipeline.
|
Hey @sambhavgupta0705, the PR is ready for review. |
princerajpoot20
changed the title
|
@princerajpoot20 have a look at nitpick comments once |
|
@princerajpoot20 can you please also add the UI preview of your local host |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/build-tools.test.ts (2)
94-104: Consider deeper assertions for the manual build test.The test validates that categories exist but doesn't verify the actual content or structure of the combined tools. Consider asserting specific properties like tool counts, descriptions, or merged data to ensure the manual build properly combines both sources.
Example enhancement:
expect(combinedToolsContent).toHaveProperty('Category1'); expect(combinedToolsContent).toHaveProperty('Category2'); +expect(combinedToolsContent.Category1.description).toEqual(mockConvertedData.Category1.description); +expect(combinedToolsContent.Category1.toolsList).toBeDefined();
106-120: Consider improving mock cleanup reliability.The spy restoration at line 119 only executes if the test passes. If the assertion fails, the spy remains mocked for subsequent tests. While
jest.clearAllMocks()inbeforeEachmay mitigate this, consider moving the restore to anafterEachblock or using a try-finally pattern for more robust cleanup.Alternative pattern:
it('should handle combineTools error', async () => { mockedAxios.get.mockResolvedValue({ data: mockExtractData }); const combineSpy = jest .spyOn(require('../scripts/tools/combine-tools'), 'combineTools') .mockRejectedValueOnce(new Error('Combine error')); - await expect( - buildTools(automatedToolsPath, manualToolsPath, toolsPath, tagsPath) - ).rejects.toThrow( - 'An error occurred while building tools: An error occurred while combining tools: Combine error' - ); - - combineSpy.mockRestore(); + try { + await expect( + buildTools(automatedToolsPath, manualToolsPath, toolsPath, tagsPath) + ).rejects.toThrow( + 'An error occurred while building tools: An error occurred while combining tools: Combine error' + ); + } finally { + combineSpy.mockRestore(); + } });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/build-tools.ts(3 hunks)tests/build-tools.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/build-tools.ts
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: asyncapi-bot
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch
📚 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-tools.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/build-tools.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-tools.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/build-tools.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/build-tools.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-tools.test.ts
🧬 Code graph analysis (1)
tests/build-tools.test.ts (2)
tests/fixtures/buildToolsData.ts (3)
mockConvertedData(79-79)manualTools(79-79)mockExtractData(79-79)scripts/build-tools.ts (2)
buildToolsManual(110-110)buildTools(110-110)
🔇 Additional comments (1)
tests/build-tools.test.ts (1)
122-144: Excellent error handling coverage for buildToolsManual.The three error tests comprehensively cover different failure scenarios:
- General filesystem errors (non-existent parent directory)
- Specific validation failure for missing automated tools
- Specific validation failure for missing manual tools
This ensures the new function properly validates inputs and provides clear error messages.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/build-tools.test.ts (1)
94-150: Consider adding tests for additional edge cases.While the current test coverage is solid, you might want to add tests for these scenarios to catch potential issues early:
- Invalid JSON in automated or manual tools files (malformed data)
- Empty objects or arrays in tool files
- Corrupted file content
These would help ensure robust error handling across more failure modes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/build-tools.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: asyncapi-bot
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch
Learnt from: akshatnema
Repo: asyncapi/website PR: 3136
File: scripts/tools/combine-tools.js:122-125
Timestamp: 2024-11-01T12:49:32.805Z
Learning: In `scripts/tools/combine-tools.js`, the existing URL parsing logic for `repoUrl` without additional error handling is acceptable and does not require changes.
📚 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-tools.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/build-tools.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-tools.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/build-tools.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-tools.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/build-tools.test.ts
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
- GitHub Check: Test NodeJS PR - windows-latest
- GitHub Check: Test NodeJS PR - macos-13
🔇 Additional comments (3)
tests/build-tools.test.ts (3)
94-108: LGTM! Well-structured test for the manual build flow.The test properly validates the happy path for
buildToolsManual, with clear setup, execution, and assertions covering the combined output structure.
128-150: No issues found—error messages correctly implemented.All three error message assertions in the test file match the implementation:
- Line 133 assertion matches line 93 of
scripts/build-tools.ts- Line 141 assertion matches line 81 of
scripts/build-tools.ts- Line 149 assertion matches line 85 of
scripts/build-tools.tsJest's
.toThrow()performs partial string matching, so the tests will pass correctly as written.
110-126: The spy implementation has a module resolution issue that requires verification.The test uses
jest.spyOn(require('../scripts/tools/combine-tools'), 'combineTools')to mockcombineTools, butbuildTools.tsimports it statically with ES6:import { combineTools } from './tools/combine-tools'at the top of the file. Withoutjest.mock('../scripts/tools/combine-tools')orjest.resetModules(), the spy on the dynamically required module won't intercept the call made by the statically imported module reference.The error message format is correct and matches the expected wrapping behavior. However, verify that:
- The spy is actually intercepting the
combineToolscall, or- Add
jest.mock('../scripts/tools/combine-tools')at the top of the test file and adjust the spy accordingly, or- Use
jest.resetModules()inbeforeEach()to ensure fresh module loading for each test
|
@sambhavgupta0705 Done |
github-project-automation
bot
moved this from In Progress
to Completed
in Bounty Program
4 participants
Approach
buildToolsManualfunction, which is attached to thebuildscript. This function combines themanual toolsandautomated tools.manual filewill now be visible in the temporary deployment preview provided by Netlify.This happens because the buildToolsManual function is triggered during the build process, and the new combined tools file reflects those changes.
Dev Testing Evidence
tools.json.Related issue(s)
Resolves #2124
Summary by CodeRabbit
Summary by CodeRabbit