test: add comprehensive test suite in conditionalGeneration.js#2030
test: add comprehensive test suite in conditionalGeneration.js#2030shivansh-source wants to merge 2 commits intoasyncapi:masterfrom
Conversation
Signed-off-by: shivansh-source <shivanshsiddhi1234@gmail.com>
shivansh-source
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.
|
📝 WalkthroughWalkthroughAdded a Jest test suite that exercises 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)
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 (4)
apps/generator/test/conditionalGeneration.test.js (4)
293-295: Remove extraneous empty comment line.✏️ Cleanup
// validateStatus – missing validate function (the "silent false" bug guard) - // describe('validateStatus – missing validate function', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/generator/test/conditionalGeneration.test.js` around lines 293 - 295, Remove the stray empty comment line immediately above the describe block for "validateStatus – missing validate function" so the comment block is concise; update the comment sequence that currently reads "// validateStatus – missing validate function" followed by an empty "// " to just the single comment line before the describe('validateStatus – missing validate function', () => { ... }) block.
33-42: Add missing JSDoc parameter documentation.Per coding guidelines, JSDoc should include parameter types. This helper mirrors
makeConditionalGenerationConfigbut lacks the@paramtags.📝 Suggested JSDoc addition
/** * Build a minimal templateConfig with conditionalFiles (deprecated API). + * + * `@param` {string} path - The matched condition path key + * `@param` {object} conditionOpts - Any subset of { subject, parameter, validate } */ function makeConditionalFilesConfig(path, conditionOpts) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/generator/test/conditionalGeneration.test.js` around lines 33 - 42, The JSDoc for makeConditionalFilesConfig is missing `@param` annotations; update the comment above the function makeConditionalFilesConfig to include `@param` {string} path and `@param` {Record<string, any> | object} conditionOpts (or the same types used by makeConditionalGenerationConfig) and a short description for each parameter so the helper mirrors the existing JSDoc style and types used by makeConditionalGenerationConfig.
1-5: Fix JSDoc formatting inconsistency.Lines 3-4 have excessive leading whitespace that breaks the alignment of the documentation block.
✏️ Suggested fix
/** * Comprehensive unit tests for lib/conditionalGeneration.js - * Focuses on the isGenerationConditionMet function which determines whether a given file path should be generated based on the template configuration and AsyncAPI document. * Covers various scenarios including parameter-based conditions, subject-based conditions using JMESPath, and the deprecated conditionalFiles API. - * Mocks loglevel and logMessages to verify correct logging behavior for matched conditions and not-generated cases. + * Focuses on the isGenerationConditionMet function which determines whether + * a given file path should be generated based on the template configuration + * and AsyncAPI document. + * Covers various scenarios including parameter-based conditions, subject-based + * conditions using JMESPath, and the deprecated conditionalFiles API. + * Mocks loglevel and logMessages to verify correct logging behavior for matched + * conditions and not-generated cases. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/generator/test/conditionalGeneration.test.js` around lines 1 - 5, Adjust the JSDoc block at the top of the test file so the comment lines are consistently aligned: remove the excessive leading whitespace on the third and fourth lines so each line starts with " * " (matching the standard JSDoc alignment used around the description of isGenerationConditionMet), ensuring the doc block reads as a contiguous, properly formatted comment describing the tests for isGenerationConditionMet and related scenarios.
190-199: Misleading test description.The test name claims "does not include server key" but looking at the implementation (context snippet 2, lines 113-116), the
serverkey is always included in the JMESPath search object—it's just set toundefinedwhentemplateParams.serveris absent. The test passes because the subject'channels."/user"'resolves independently of the server key.Consider updating the description for accuracy:
📝 Suggested description update
- it('does not include server key when templateParams.server is absent', async () => { + it('sets server to undefined in JMESPath object when templateParams.server is absent', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/generator/test/conditionalGeneration.test.js` around lines 190 - 199, The test title is misleading: the code under test (isGenerationConditionMet called with config from makeConditionalGenerationConfig) always includes a server field in the JMESPath search object (possibly undefined) so the assertion is actually verifying that the subject 'channels."/user"' resolves regardless of templateParams.server; update the test description to reflect that behavior (e.g., "skips server-dependent condition when templateParams.server is absent" or "does not require templateParams.server for channel subject resolution") so it accurately describes what is being validated by makeAsyncapiDocument, makeConditionalGenerationConfig, PATH and isGenerationConditionMet.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/generator/test/conditionalGeneration.test.js`:
- Around line 43-50: The helper makeAsyncapiDocument creates fresh mocks each
time servers() is called which breaks call assertions; change it to create and
reuse a single serversMock object (with a persistent get mock that returns
either a serverMock or undefined) and have servers() return that same
serversMock so expect(doc.servers().get).toHaveBeenCalledWith('myServer')
targets the same mock; update makeAsyncapiDocument to construct a serverMock and
serversMock once (reused across calls) and ensure json() mocks still return the
provided jsonData/serverData, and also add JSDoc above makeAsyncapiDocument
describing parameters and return shape per project guidelines.
---
Nitpick comments:
In `@apps/generator/test/conditionalGeneration.test.js`:
- Around line 293-295: Remove the stray empty comment line immediately above the
describe block for "validateStatus – missing validate function" so the comment
block is concise; update the comment sequence that currently reads "//
validateStatus – missing validate function" followed by an empty "// " to just
the single comment line before the describe('validateStatus – missing validate
function', () => { ... }) block.
- Around line 33-42: The JSDoc for makeConditionalFilesConfig is missing `@param`
annotations; update the comment above the function makeConditionalFilesConfig to
include `@param` {string} path and `@param` {Record<string, any> | object}
conditionOpts (or the same types used by makeConditionalGenerationConfig) and a
short description for each parameter so the helper mirrors the existing JSDoc
style and types used by makeConditionalGenerationConfig.
- Around line 1-5: Adjust the JSDoc block at the top of the test file so the
comment lines are consistently aligned: remove the excessive leading whitespace
on the third and fourth lines so each line starts with " * " (matching the
standard JSDoc alignment used around the description of
isGenerationConditionMet), ensuring the doc block reads as a contiguous,
properly formatted comment describing the tests for isGenerationConditionMet and
related scenarios.
- Around line 190-199: The test title is misleading: the code under test
(isGenerationConditionMet called with config from
makeConditionalGenerationConfig) always includes a server field in the JMESPath
search object (possibly undefined) so the assertion is actually verifying that
the subject 'channels."/user"' resolves regardless of templateParams.server;
update the test description to reflect that behavior (e.g., "skips
server-dependent condition when templateParams.server is absent" or "does not
require templateParams.server for channel subject resolution") so it accurately
describes what is being validated by makeAsyncapiDocument,
makeConditionalGenerationConfig, PATH and isGenerationConditionMet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6540435-36ce-4ee7-bd56-620c606e9254
📒 Files selected for processing (1)
apps/generator/test/conditionalGeneration.test.js
Signed-off-by: shivansh-source <shivanshsiddhi1234@gmail.com>
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/generator/test/conditionalGeneration.test.js (2)
31-40: Add complete JSDoc documentation for consistency.The
makeConditionalFilesConfighelper is missing@paramand@returnstags, andmakeConditionalGenerationConfigis missing the@returnstag. For consistency withmakeAsyncapiDocument, consider adding complete documentation.📝 Proposed JSDoc additions
/** * Build a minimal templateConfig with conditionalGeneration entries. * * `@param` {string} path - The matched condition path key * `@param` {object} conditionOpts - Any subset of { subject, parameter, validate } + * `@returns` {object} A templateConfig object with conditionalGeneration */ function makeConditionalGenerationConfig(path, conditionOpts) {/** * Build a minimal templateConfig with conditionalFiles (deprecated API). + * + * `@param` {string} path - The matched condition path key + * `@param` {object} conditionOpts - Any subset of { subject, parameter, validate } + * `@returns` {object} A templateConfig object with conditionalFiles */ function makeConditionalFilesConfig(path, conditionOpts) {As per coding guidelines: "Write clear and consistent JSDoc comments for functions, including parameter types, return values, and error conditions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/generator/test/conditionalGeneration.test.js` around lines 31 - 40, Update the JSDoc for the helper functions to include complete tags: add `@param` and `@returns` to makeConditionalFilesConfig (documenting the path: string and conditionOpts: object, and that it returns an object with a conditionalFiles map), and add a `@returns` tag to makeConditionalGenerationConfig describing its return shape; target the function names makeConditionalFilesConfig and makeConditionalGenerationConfig and ensure types and brief descriptions match existing style used by makeAsyncapiDocument.
285-293: Test name claims "silent" behavior but doesn't verify it.This test is named "(silent false)" implying no logging occurs, but it doesn't assert that
log.debugwas not called. The test at lines 323-329 tests the identical scenario and properly verifies the silent behavior withexpect(log.debug).not.toHaveBeenCalled().Consider either removing this duplicate test or adding the missing assertion for consistency.
♻️ Option 1: Add the missing assertion
it('returns false when conditionalFiles validate is missing (silent false)', async () => { const doc = makeAsyncapiDocument({ info: { title: 'Test' } }); const config = makeConditionalFilesConfig(PATH, { subject: 'info.title', // no validate }); const result = await isGenerationConditionMet(config, PATH, {}, doc); expect(result).toBe(false); + expect(log.debug).not.toHaveBeenCalled(); });♻️ Option 2: Remove this test (covered by lines 323-329)
The test at lines 323-329 already covers this exact scenario with the proper silence assertion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/generator/test/conditionalGeneration.test.js` around lines 285 - 293, The test "returns false when conditionalFiles validate is missing (silent false)" duplicates behavior already covered later but is missing the silence assertion; update this test (the one calling isGenerationConditionMet with config from makeConditionalFilesConfig and doc from makeAsyncapiDocument) to include expect(log.debug).not.toHaveBeenCalled() after the existing expect(result).toBe(false), or alternatively remove this duplicate test entirely since the identical scenario is asserted with silence in the later test that already verifies log.debug was not called.
🤖 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/test/conditionalGeneration.test.js`:
- Around line 31-40: Update the JSDoc for the helper functions to include
complete tags: add `@param` and `@returns` to makeConditionalFilesConfig
(documenting the path: string and conditionOpts: object, and that it returns an
object with a conditionalFiles map), and add a `@returns` tag to
makeConditionalGenerationConfig describing its return shape; target the function
names makeConditionalFilesConfig and makeConditionalGenerationConfig and ensure
types and brief descriptions match existing style used by makeAsyncapiDocument.
- Around line 285-293: The test "returns false when conditionalFiles validate is
missing (silent false)" duplicates behavior already covered later but is missing
the silence assertion; update this test (the one calling
isGenerationConditionMet with config from makeConditionalFilesConfig and doc
from makeAsyncapiDocument) to include expect(log.debug).not.toHaveBeenCalled()
after the existing expect(result).toBe(false), or alternatively remove this
duplicate test entirely since the identical scenario is asserted with silence in
the later test that already verifies log.debug was not called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1c229222-1f9f-4590-9793-8710a2e451b1
📒 Files selected for processing (1)
apps/generator/test/conditionalGeneration.test.js
2 participants
description : This PR adds a dedicated test suite for lib/conditionalGeneration.js. Despite being a critical component for conditional file and folder generation, this module previously had 0% test coverage. These tests ensure the complex logic for evaluating template configurations remains stable and predictable.
The newly implemented tests provide thorough validation across four primary areas. First, it ensures proper guard clauses by verifying that the utility returns undefined when no configuration is present. For parameter-based generation, the suite covers pass/fail scenarios, missing validation fields, and the correct forwarding of values. For subject-based generation, the tests validate JMESPath resolution, the merging of templateParams.server into the evaluation context, and the performance-critical requirement that asyncapiDocument.json() is only invoked on subject paths. Finally, the suite includes specific coverage for the deprecated conditionalFiles configuration, confirming it triggers the appropriate warning logs and correctly takes precedence over conditionalGeneration during this transition period. This foundational coverage is essential for mitigating regression risks and improving maintainability as we move toward the refactor planned in issue #1553.
fixes #1970
Summary by CodeRabbit