test: add unit tests for scripts/compose.ts#5626
Conversation
codxbrexx
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg,
princerajpoot20 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5626 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 22 24 +2
Lines 796 998 +202
Branches 146 198 +52
==========================================
+ Hits 796 998 +202 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5626--asyncapi-website.netlify.app/ |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe compose script now exports slug, front-matter, and post-writing helpers, separates CLI execution from file creation, removes the script from Jest coverage exclusions, and adds deterministic tests for serialization, slugging, writing, logging, and errors. ChangesCompose script refactor and coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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/compose.ts (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: prefer the
node:protocol import.SonarCloud flags
import ... from 'url';node:urlmakes the built-in explicit. Purely stylistic.♻️ Proposed change
-import { fileURLToPath } from 'url'; +import { fileURLToPath } from 'node:url';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/compose.ts` at line 9, Update the fileURLToPath import in scripts/compose.ts to use the explicit node:url protocol instead of url. No other changes are needed.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/compose.ts`:
- Around line 202-204: Return the Promise from writePost(answers) within the
.then callback so its rejection propagates to the existing .catch handler; do
not leave the asynchronous call unreturned.
---
Nitpick comments:
In `@scripts/compose.ts`:
- Line 9: Update the fileURLToPath import in scripts/compose.ts to use the
explicit node:url protocol instead of url. No other changes are needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d0986d7-3a40-4596-8d3e-79db12e92055
📒 Files selected for processing (3)
jest.config.jsscripts/compose.tstests/scripts/compose.test.ts
|
I've added the missing scripts/compose.ts tests. Could you please review the PR and let me know if anything else needs to be changed or improved? |
princerajpoot20
left a comment
princerajpoot20
left a comment
There was a problem hiding this comment.
What is the reason for making changes in the scripts folder when the objective was only to add tests? I was expecting the changes to be limited to the test files.
Also, Sonar has reported two issues. Please have a look at it.
…in, and improve error handling for CLI execution
The original file runs inquirer.prompt() at the top level, so Jest hangs the moment it imports the file — making it impossible to test. The refactor wraps the CLI in main() with a process.argv guard (same pattern as build-meetings.ts), with zero change to runtime behavior. The tests cannot exist without it. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/compose.ts (2)
175-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
try/catchover mixedasync/await+.then/.catch.
main()is declaredasyncbut uses.then().catch()chaining instead ofawaitwithtry/catch. This mixes two paradigms unnecessarily and reduces readability. Refactoring to pureasync/awaitwithtry/catchis more idiomatic and easier to maintain.♻️ Proposed refactor
async function main(): Promise<void> { - await inquirer - .prompt([ + try { + const answers = await inquirer.prompt([ { name: 'title', message: 'Enter post title:', type: 'input' }, { name: 'excerpt', message: 'Enter post excerpt:', type: 'input' }, { name: 'tags', message: 'Any Tags? Separate them with , or leave empty if no tags.', type: 'input' }, { name: 'type', message: 'Enter the post type:', type: 'list', choices: ['Communication', 'Community', 'Engineering', 'Marketing', 'Strategy', 'Video'] }, { name: 'canonical', message: 'Enter the canonical URL if any:', type: 'input' } - ]) - .then((answers: ComposePromptType) => { - return writePost(answers); - }) - .catch((error) => { + ]); + await writePost(answers); + } catch (error) { logger.error(error); if (error.isTtyError) { logger.error("Prompt couldn't be rendered in the current environment"); } else { logger.error('Something went wrong, sorry!'); } - }); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/compose.ts` around lines 175 - 215, Refactor main() to use a try/catch around the awaited inquirer.prompt call, then pass the resulting answers to writePost with await. Remove the chained .then() and .catch() calls while preserving the existing logger.error handling, including the isTtyError-specific message and fallback error message.
220-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider top-level await as an alternative to
main().catch().SonarCloud flags this line suggesting top-level await. Since the module already uses ESM (
import.meta.url), top-level await is available and would simplify the entry-point error handling. This is optional — the current pattern is safe and widely used.♻️ Optional refactor
- main().catch((error) => { - logger.error(error); - }); + try { + await main(); + } catch (error) { + logger.error(error); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/compose.ts` around lines 220 - 222, Optionally replace the main().catch entry-point in scripts/compose.ts with top-level await, preserving the existing logger.error handling for failures. Since main is already the entry function and the module uses ESM, keep behavior unchanged while simplifying the error-handling flow.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/compose.ts`:
- Around line 175-215: Refactor main() to use a try/catch around the awaited
inquirer.prompt call, then pass the resulting answers to writePost with await.
Remove the chained .then() and .catch() calls while preserving the existing
logger.error handling, including the isTtyError-specific message and fallback
error message.
- Around line 220-222: Optionally replace the main().catch entry-point in
scripts/compose.ts with top-level await, preserving the existing logger.error
handling for failures. Since main is already the entry function and the module
uses ESM, keep behavior unchanged while simplifying the error-handling flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aae553c0-3ad6-42c1-9924-d4335bf98c8d
📒 Files selected for processing (1)
scripts/compose.ts
…ions in compose script
|
3 participants
Description
scripts/compose.tsto exportgetSlug(),genFrontMatter(), andwritePost()as named functions, and guarded theinquirerCLI behind amain()function with aprocess.argvcheck (same pattern used bybuild-meetings.ts) — no change to runtime behaviortests/scripts/compose.test.tswith 25 tests covering slug generation (kebab-case, special characters, edge cases), front matter output (title fallback, tag parsing, date format, canonical URL), and file writing (success path, untitled fallback, error handling)scripts/compose.tsfromcoveragePathIgnorePatternsinjest.config.jsso coverage is now tracked for this fileRelated issue(s)
Resolves #5096
Summary by CodeRabbit
Improvements
Tests
Chores