test: add unit tests for baked-in template discovery and filtering#1943
test: add unit tests for baked-in template discovery and filtering#1943miantalha45 wants to merge 3 commits intoasyncapi:masterfrom
Conversation
miantalha45
requested review from
asyncapi-bot-eve,
derberg,
jonaslagoni and
magicmatatjahu
as code owners
|
What reviewer looks at during PR reviewThe following are ideal points maintainers look for during review. Reviewing these points yourself beforehand can help streamline the review process and reduce time to merge.
|
📝 WalkthroughWalkthroughAdds a Jest test suite and a fixture for baked‑in template helpers; updates getTemplate to validate inputs and return undefined for invalid or missing templates, and to prefer an explicit template.path with a fallback resolution. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
🤖 Fix all issues with AI agents
In `@apps/generator/test/bakedInTemplates.test.js`:
- Around line 201-219: The tests currently rely on incidental TypeErrors; add
explicit validation in getTemplate to check the input (e.g.,
null/undefined/empty string) and throw a clear, intentional error (for example
throw new Error('Invalid template name') ) and then update the tests in
bakedInTemplates.test.js to assert that specific error/message (use await
expect(getTemplate(...)).rejects.toThrow('Invalid template name')) so the
contract is explicit and resilient to implementation changes.
🧹 Nitpick comments (3)
apps/generator/test/bakedInTemplates.test.js (3)
112-119: Conditional guard causes this test to silently pass without asserting anything.If no templates lack a
stackproperty, theifblock is skipped and the test passes vacuously. This is a common testing anti-pattern — a green check mark with zero coverage.Consider either: (a) removing the guard and asserting unconditionally (skip the test if the precondition isn't met), or (b) using a mock/fixture to guarantee the precondition.
Option A: Skip when precondition is unmet (preferred)
it('handles templates without optional stack property', () => { const templatesWithoutStack = templates.filter(t => !t.stack); - if (templatesWithoutStack.length > 0) { - const result = listBakedInTemplates({ stack: 'quarkus' }); - // Should not include templates without stack property - expect(result.every(t => t.stack === 'quarkus')).toBe(true); - } + // Ensure precondition is met so the test is meaningful + expect(templatesWithoutStack.length).toBeGreaterThan(0); + const result = listBakedInTemplates({ stack: 'quarkus' }); + // Filtered results must not include templates without a stack property + expect(result.every(t => t.stack === 'quarkus')).toBe(true); + // None of the stack-less templates should appear + templatesWithoutStack.forEach(t => { + expect(result.find(r => r.name === t.name)).toBeUndefined(); + }); });
150-156: Same silent-pass issue with theifguard on case-sensitivity test.If
templates[0].nameis already all-uppercase, theifis skipped and nothing is asserted. This test should guarantee its precondition or use a known fixture.Proposed fix
it('is case-sensitive', () => { const existingTemplate = templates[0]; const caseVariation = existingTemplate.name.toUpperCase(); - if (caseVariation !== existingTemplate.name) { - expect(isCoreTemplate(caseVariation)).toBe(false); - } + // Ensure the precondition holds so the test is meaningful + expect(caseVariation).not.toBe(existingTemplate.name); + expect(isCoreTemplate(caseVariation)).toBe(false); });
170-189: Two moreif-guarded tests that can silently pass.Both "uses template path when available" (Line 172) and "falls back to bakedInTemplates directory" (Line 180) are fully wrapped in conditionals. If the JSON data doesn't contain the expected shape, these tests contribute nothing. Apply the same fix: assert the precondition up front or use controlled fixtures.
| it('throws error for non-existing template', async () => { | ||
| // Current implementation throws when template.name is accessed on undefined | ||
| await expect(getTemplate('nonexistent-template')).rejects.toThrow(); | ||
| }); | ||
|
|
||
| it('throws error for empty string template name', async () => { | ||
| // Current implementation will throw when template is not found | ||
| await expect(getTemplate('')).rejects.toThrow(); | ||
| }); | ||
|
|
||
| it('throws error for null template name', async () => { | ||
| // Current implementation will throw when template is not found | ||
| await expect(getTemplate(null)).rejects.toThrow(); | ||
| }); | ||
|
|
||
| it('throws error for undefined template name', async () => { | ||
| // Current implementation will throw when template is not found | ||
| await expect(getTemplate(undefined)).rejects.toThrow(); | ||
| }); |
There was a problem hiding this comment.
Error tests rely on incidental TypeError rather than intentional error handling.
The comments on Lines 202, 207, 212 acknowledge these tests depend on the implementation accidentally throwing (e.g., accessing .name on undefined). This is brittle — if the implementation changes to return undefined or null instead of throwing, these tests break or become misleading.
Consider verifying the actual error type/message to document the expected contract, or filing an issue to add explicit validation in getTemplate.
#!/bin/bash
# Check if getTemplate has explicit validation/error handling for invalid input
ast-grep --pattern $'function getTemplate($_) {
$$$
}'
# Also check as arrow/async function or module.exports
rg -n -A 15 'getTemplate' --glob '**/bakedInTemplates.js'🤖 Prompt for AI Agents
In `@apps/generator/test/bakedInTemplates.test.js` around lines 201 - 219, The
tests currently rely on incidental TypeErrors; add explicit validation in
getTemplate to check the input (e.g., null/undefined/empty string) and throw a
clear, intentional error (for example throw new Error('Invalid template name') )
and then update the tests in bakedInTemplates.test.js to assert that specific
error/message (use await expect(getTemplate(...)).rejects.toThrow('Invalid
template name')) so the contract is explicit and resilient to implementation
changes.
dcd5b05 to
ba7e988
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/generator/lib/templates/bakedInTemplates.js`:
- Line 43: Update the JSDoc on the function in
apps/generator/lib/templates/bakedInTemplates.js that currently has "@returns
{Promise<{name: string, path: string}|undefined>}" to remove the "|undefined"
since the function never returns undefined, and add one or more "@throws" tags
documenting the error conditions the function can raise (e.g., invalid
input/argument validation errors and "template not found" errors) so callers
know what exceptions to handle; keep the existing `@returns` describing
Promise<{name: string, path: string}> and ensure the `@throws` messages reference
the actual thrown error types/messages used by that function.
🧹 Nitpick comments (1)
apps/generator/lib/templates/bakedInTemplates.js (1)
53-53: Redundant__dirnameinpath.resolve—BAKED_IN_TEMPLATES_DIRis already absolute.
BAKED_IN_TEMPLATES_DIR(Line 4) is resolved to an absolute path viapath.resolve(__dirname, 'bakedInTemplates'). Whenpath.resolveencounters an absolute segment, it discards all preceding segments, so__dirnamehere is silently ignored. The code works but is misleading.Suggested fix
- const templatePath = template.path || path.resolve(__dirname, BAKED_IN_TEMPLATES_DIR, template.name); + const templatePath = template.path || path.resolve(BAKED_IN_TEMPLATES_DIR, template.name);
| @@ -43,9 +43,14 @@ module.exports.isCoreTemplate = (templateName) => { | |||
| * @returns {Promise<{name: string, path: string}|undefined>} An object containing the template's name and path, or undefined if not found. | |||
There was a problem hiding this comment.
JSDoc @returns is now inaccurate — the function never returns undefined.
Since both invalid input and missing templates now throw, the |undefined in the return type is misleading. Also, the @throws conditions should be documented per the coding guidelines.
Proposed fix
- * `@returns` {Promise<{name: string, path: string}|undefined>} An object containing the template's name and path, or undefined if not found.
+ * `@returns` {Promise<{name: string, path: string}>} An object containing the template's name and resolved path.
+ * `@throws` {Error} If templateName is not a non-empty string or does not match any core template.As per coding guidelines: **/*.{js,ts,jsx,tsx}: Write clear and consistent JSDoc comments for functions, including parameter types, return values, and error conditions.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * @returns {Promise<{name: string, path: string}|undefined>} An object containing the template's name and path, or undefined if not found. | |
| * `@returns` {Promise<{name: string, path: string}>} An object containing the template's name and resolved path. | |
| * `@throws` {Error} If templateName is not a non-empty string or does not match any core template. |
🤖 Prompt for AI Agents
In `@apps/generator/lib/templates/bakedInTemplates.js` at line 43, Update the
JSDoc on the function in apps/generator/lib/templates/bakedInTemplates.js that
currently has "@returns {Promise<{name: string, path: string}|undefined>}" to
remove the "|undefined" since the function never returns undefined, and add one
or more "@throws" tags documenting the error conditions the function can raise
(e.g., invalid input/argument validation errors and "template not found" errors)
so callers know what exceptions to handle; keep the existing `@returns` describing
Promise<{name: string, path: string}> and ensure the `@throws` messages reference
the actual thrown error types/messages used by that function.
There was a problem hiding this comment.
@miantalha45 please look at CodeRabbit, it rarely makes mistakes
| */ | ||
| const path = require('path'); | ||
| const { listBakedInTemplates, isCoreTemplate, getTemplate } = require('../lib/templates/bakedInTemplates'); | ||
| const templates = require('../lib/templates/BakedInTemplatesList.json'); |
There was a problem hiding this comment.
you cannot test using production data because any changes in the list of templates can cause false/positive errors in tests - you need to create a dummy fixture that will be used only in tests
| @@ -43,9 +43,14 @@ module.exports.isCoreTemplate = (templateName) => { | |||
| * @returns {Promise<{name: string, path: string}|undefined>} An object containing the template's name and path, or undefined if not found. | |||
| */ | |||
| module.exports.getTemplate = async (templateName) => { | |||
There was a problem hiding this comment.
I'm not fan of throwing errors but rather gracefully going forward if possible. So if template name is invalid, it should work like the template name is not on the list, and return empty array, not throw error
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/generator/lib/templates/bakedInTemplates.js (1)
54-54: Redundant__dirnameinpath.resolve— the first argument is silently ignored.
BAKED_IN_TEMPLATES_DIR(line 4) is already an absolute path produced bypath.resolve(__dirname, 'bakedInTemplates'). Whenpath.resolveencounters an absolute segment, it discards everything before it, so the leading__dirnamehere is dead code and misleading to future readers.Proposed fix
- const templatePath = template.path || path.resolve(__dirname, BAKED_IN_TEMPLATES_DIR, template.name); + const templatePath = template.path || path.resolve(BAKED_IN_TEMPLATES_DIR, template.name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/generator/lib/templates/bakedInTemplates.js` at line 54, The call constructing templatePath uses path.resolve(__dirname, BAKED_IN_TEMPLATES_DIR, template.name) but BAKED_IN_TEMPLATES_DIR is already an absolute path so the leading __dirname is redundant; update the expression that sets templatePath (where template.path || path.resolve(...)) to remove __dirname and call path.resolve(BAKED_IN_TEMPLATES_DIR, template.name) (keeping the fallback to template.path unchanged), ensuring references to templatePath, BAKED_IN_TEMPLATES_DIR, template.name, and path.resolve are adjusted accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/generator/lib/templates/bakedInTemplates.js`:
- Line 54: The call constructing templatePath uses path.resolve(__dirname,
BAKED_IN_TEMPLATES_DIR, template.name) but BAKED_IN_TEMPLATES_DIR is already an
absolute path so the leading __dirname is redundant; update the expression that
sets templatePath (where template.path || path.resolve(...)) to remove __dirname
and call path.resolve(BAKED_IN_TEMPLATES_DIR, template.name) (keeping the
fallback to template.path unchanged), ensuring references to templatePath,
BAKED_IN_TEMPLATES_DIR, template.name, and path.resolve are adjusted
accordingly.
3 participants
Description
Adds comprehensive unit tests for the baked-in template discovery and filtering functionality in
lib/templates/bakedInTemplates.js. This addresses the gap where core template discovery functions had no dedicated test coverage.What's Changed
test/bakedInTemplates.test.jswith 31 test cases covering:listBakedInTemplates: 16 tests covering no filter, single filters (type, stack, protocol, target), combined filters, and edge casesisCoreTemplate: 7 tests covering existing/non-existing templates and edge cases (null, undefined, empty string, case sensitivity)getTemplate: 8 tests covering path resolution, fallback behavior, and error handlingTesting
All 31 tests pass:
Related Issue
Fixes #1850
Checklist
Summary by CodeRabbit