test: add unit tests for modelspresets.js #1950
test: add unit tests for modelspresets.js #1950asyncapi-bot merged 8 commits intoasyncapi:masterfrom
Conversation
|
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.
|
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
📝 WalkthroughWalkthroughAdds a new unit test suite for 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)
Tip We've launched Issue Planner and it is currently in beta. Please try it out and 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 |
Adi-204
left a comment
There was a problem hiding this comment.
@Ishita-190 the testing are failing because of linting issue. The linting error is from SonarJS, which is complaining about duplicate string literals. I think it should be solve by defining constants at the top for all repeated strings. you can run npm run lint locally to see whether it is fix or not after changes it should pass. I hope it helps feel free to ask questions!
|
@Adi-204 thanks for pointing this out! I've made the changes, and ran all the tests again and everything seems to be running well. Let me know if any other changes need to be made. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/helpers/test/ModelsPresets.test.js`:
- Around line 85-93: The test exposes that calling
Object.values(model.properties).forEach in the class preset will throw when
model.properties is undefined; to fix, update the source where that loop runs to
guard against null/undefined (e.g., replace
Object.values(model.properties).forEach(...) with Object.values(model.properties
|| {}).forEach(...)) and then adjust the test using websocketPreset.class.self
to expect no throw (keep the test asserting PACKAGE_DECLARATION, IMPORT_OBJECTS
and the class content); alternatively, if you prefer the error, change the test
to expect a TypeError via expect(() =>
websocketPreset.class.self(...)).toThrow(TypeError).
🧹 Nitpick comments (1)
packages/helpers/test/ModelsPresets.test.js (1)
39-39: Nit:websocketPresetis declared identically in threedescribeblocks.You could hoist it to the top-level
describe('ModelsPresets', ...)scope to reduce repetition. That said, keeping it local to each block is also a valid readability choice.Also applies to: 97-97, 115-115
Adi-204
left a comment
There was a problem hiding this comment.
lgtm! @Ishita-190 can you please update PR description and add screenshot of test coverage
something like ss attach in PR #1952
|
@Adi-204 All done, please check! |
|
|
/rtm |



Fixes: #1905
Description
Added comprehensive unit tests for the
ModelsPresets.jscomponent in the helpers package.test/ModelsPresets.test.jswith 12 test casesSummary by CodeRabbit