test: add tests for Readme component#1953
Conversation
batchu5
requested review from
Adi-204,
asyncapi-bot-eve,
derberg 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 new test suite Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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: 2
🤖 Fix all issues with AI agents
In `@packages/components/test/components/Readme.test.js`:
- Around line 14-25: The test reuses JS-specific params (params built via
buildParams('javascript', { clientFileName: 'myClient.js' }, 'production')) for
the Python snapshot, causing mismatched output; update the test to build
language-appropriate params inside the languages.forEach loop (or rebuild per
iteration) using buildParams(language, { clientFileName:
appropriateExtensionFor(language) }, 'production') so each iteration uses a
correct clientFileName/params for the current language variable rather than the
single params created in beforeAll.
- Around line 78-89: Add explicit validation in the Readme component to check
the incoming language prop against the supported languages set (the same source
used by Usage/usageConfig and CoreMethods) and throw a clear Error like
"Unsupported language: <language>" immediately if it's not supported; do this
before rendering child components (so Usage's lookup won't run on undefined).
Reference the Readme component's render/functional entry point and the supported
languages source (usageConfig / CoreMethods supported list) to perform the
check, and also ensure Installation.js receives a validated language or a safe
fallback so it doesn't render "undefined".
🧹 Nitpick comments (1)
packages/components/test/components/Readme.test.js (1)
42-141: Consider consolidating repetitive negative tests withtest.each.The seven error-case tests follow an identical pattern (render with bad props → expect TypeError). Using
test.eachwould reduce duplication and make it easier to add new cases.Example consolidation
- test('render Readme component when asyncapi is missing', () => { - const renderReadme = () => - render( - <Readme - params={params} - language="javascript" - /> - ); - - expect(renderReadme).toThrow(TypeError); - }); - - test('render Readme component when params are missing', () => { ... }); - // ... etc. + test.each([ + ['asyncapi is missing', { params, language: 'javascript' }], + ['params are missing', { asyncapi: parsedAsyncAPIDocument, language: 'javascript' }], + ['language is missing', { asyncapi: parsedAsyncAPIDocument, params }], + ['asyncapi is null', { asyncapi: null, params, language: 'javascript' }], + ['params is null', { asyncapi: parsedAsyncAPIDocument, params: null, language: 'javascript' }], + ['language is null', { asyncapi: parsedAsyncAPIDocument, params, language: null }], + ['language is empty', { asyncapi: parsedAsyncAPIDocument, params, language: '' }], + ['language is unsupported',{ asyncapi: parsedAsyncAPIDocument, params, language: 'dart' }], + ])('throws TypeError when %s', (_label, props) => { + expect(() => render(<Readme {...props} />)).toThrow(TypeError); + });Note: Since
parsedAsyncAPIDocumentandparamsare set inbeforeAll, you'd need to define the cases inside thedescribeblock or use a factory function.
| test('render Readme with unsupported language', () => { | ||
| const renderReadme = () => | ||
| render( | ||
| <Readme | ||
| asyncapi={parsedAsyncAPIDocument} | ||
| params={params} | ||
| language='dart' | ||
| /> | ||
| ); | ||
|
|
||
| expect(renderReadme).toThrow(TypeError); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find component files
echo "=== Searching for Installation component ==="
fd -i installation . --type f | head -20
echo -e "\n=== Searching for Usage component ==="
fd -i usage . --type f | head -20
echo -e "\n=== Searching for CoreMethods component ==="
fd -i coremethods . --type f | head -20
echo -e "\n=== Searching for Readme component ==="
fd -i readme . --type f | head -20Repository: asyncapi/generator
Length of output: 1543
🏁 Script executed:
#!/bin/bash
# Check the Readme component first
echo "=== Readme.js component ==="
cat ./packages/components/src/components/readme/Readme.js
echo -e "\n=== Installation.js component ==="
cat ./packages/components/src/components/readme/Installation.js
echo -e "\n=== Usage.js component ==="
cat ./packages/components/src/components/readme/Usage.js
echo -e "\n=== CoreMethods.js component ==="
cat ./packages/components/src/components/readme/CoreMethods.jsRepository: asyncapi/generator
Length of output: 7612
🏁 Script executed:
#!/bin/bash
# Verify the test file to understand the full context
cat -n ./packages/components/test/components/Readme.test.js | sed -n '70,95p'Repository: asyncapi/generator
Length of output: 826
🏁 Script executed:
#!/bin/bash
# Check if there's an empty string language test mentioned in the review
cat -n ./packages/components/test/components/Readme.test.js | grep -A 10 "empty string"Repository: asyncapi/generator
Length of output: 418
🏁 Script executed:
#!/bin/bash
# Check the complete test file to understand the context of all language tests
wc -l ./packages/components/test/components/Readme.test.js
echo ""
cat -n ./packages/components/test/components/Readme.test.js | head -150Repository: asyncapi/generator
Length of output: 4525
Add explicit language validation to the Readme component instead of relying on child component errors.
The TypeError is thrown, but not by explicit validation—it's an accidental side effect of Usage.js trying to call usageConfig[language] as a function when it's undefined. The CoreMethods.js component explicitly validates and throws a proper Error for unsupported languages, but it's never reached because Usage fails first. Additionally, Installation.js silently renders "undefined" without any validation. This creates fragile, implicit error handling that depends on component render order. Add explicit language validation with a clear error message at the Readme level before passing to child components.
🤖 Prompt for AI Agents
In `@packages/components/test/components/Readme.test.js` around lines 78 - 89, Add
explicit validation in the Readme component to check the incoming language prop
against the supported languages set (the same source used by Usage/usageConfig
and CoreMethods) and throw a clear Error like "Unsupported language: <language>"
immediately if it's not supported; do this before rendering child components (so
Usage's lookup won't run on undefined). Reference the Readme component's
render/functional entry point and the supported languages source (usageConfig /
CoreMethods supported list) to perform the check, and also ensure
Installation.js receives a validated language or a safe fallback so it doesn't
render "undefined".
| const params = buildParams( | ||
| language, | ||
| { clientFileName }, | ||
| 'production' | ||
| ); |
There was a problem hiding this comment.
I would not recommend using this helper buildParams. As in actual code of Readme we don't use this helper. Additionally if the helper changes all other test will start failing and this helper has nothing to do with Readme component. Just use directly object.
| test.each([ | ||
| ['asyncapi is missing', { params, language: 'javascript' }], | ||
| ['params are missing', { asyncapi: parsedAsyncAPIDocument, language: 'javascript' }], | ||
| ['language is missing', { asyncapi: parsedAsyncAPIDocument, params }], | ||
| ['asyncapi is null', { asyncapi: null, params, language: 'javascript' }], | ||
| ['params is null', { asyncapi: parsedAsyncAPIDocument, params: null, language: 'javascript' }], | ||
| ['language is null', { asyncapi: parsedAsyncAPIDocument, params, language: null }], | ||
| ['language is empty', { asyncapi: parsedAsyncAPIDocument, params, language: '' }], | ||
| ['language is unsupported',{ asyncapi: parsedAsyncAPIDocument, params, language: 'dart' }], | ||
| ])('throws TypeError when %s', (_label, props) => { | ||
| expect(() => render(<Readme {...props} />)).toThrow(TypeError); | ||
| }); |
There was a problem hiding this comment.
remove this testcase for now no need!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/components/test/components/Readme.test.js`:
- Around line 9-42: Add the missing negative test that asserts Readme throws
when a required prop is omitted: create a new test named like "throws when
required asyncapi prop is missing" that calls render with Readme but without the
asyncapi prop (provide minimal params and language) and assert that the call
throws using expect(...).toThrow(); reference the Readme component and the
render helper to locate where to add the test alongside the existing
languageConfigs tests.
🧹 Nitpick comments (1)
packages/components/test/components/Readme.test.js (1)
17-20: No guard if the fixture fails to parse.If
parseResult.documentisnull(e.g., fixture has validation errors), all tests will fail with an obscure error. A quick assertion inbeforeAllmakes failures obvious:const parseResult = await fromFile(parser, asyncapi_websocket_query).parse(); parsedAsyncAPIDocument = parseResult.document; + expect(parsedAsyncAPIDocument).toBeDefined();
|
I had included similar nitpick comments in PR #1967, but Adi recommended removing them. Following the same approach, I think we can skip them here too. |
|
|
@batchu5 always don't directly follow coderabbit suggestion, if you feel doubt just drop your comment (don't mark it resolve directly) why it is not need maintainer will verify and let you know. |
|
/rtm |
|
I’m sorry, @Adi-204, that was my mistake. I won’t repeat it next time. |
3 participants
Adds comprehensive tests for the Readme component to verify correct rendering and error behavior.
What’s included :
Readmewith all required parameters\Fixes #1940
Summary by CodeRabbit