test: add tests for bindings.js#getQueryParams #1576
Conversation
achaljhawar
requested review from
derberg,
magicmatatjahu,
jonaslagoni and
asyncapi-bot-eve
as code owners
|
|
Warning Rate limit exceeded@achaljhawar has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
""" WalkthroughA new test suite was added for the Changes
Assessment against linked issues
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Hello, @coderabbitai[bot]! 👋🏼 |
|
Tip For best results, initiate chat on the files or code changes. @asyncapi-bot Thank you for the helpful information! 🙏 I'm familiar with these commands and will keep them in mind for future interactions in this repository. The slash commands you've listed are quite useful for managing PR workflows. ( o.o ) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/helpers/test/bindings.test.js (1)
79-79: Fix formatting: Remove extra space.There's an extra space before the
itfunction call.- it('should return null if channel itself is not as expected (e.g. missing bindings function)', () => { + it('should return null if channel itself is not as expected (e.g. missing bindings function)', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/helpers/test/bindings.test.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test generator as dependency with Node 20
🔇 Additional comments (11)
packages/helpers/test/bindings.test.js (11)
1-2: Import statement looks good.The import correctly brings in the
getQueryParamsfunction from the bindings module.
3-14: Well-designed MockChannel class.The mock class effectively simulates a channel with WebSocket bindings. The
bindings()method correctly returns an object withhas()andget()methods that properly handle the 'ws' key, which aligns with the expected interface.
16-24: MockChannelsMap implementation is appropriate.The class extends
Mapand adds the necessaryisEmpty()andall()methods. The implementation correctly delegates to the underlying Map functionality.
26-31: Good test case for missing WebSocket binding.This test correctly verifies that the function returns
nullwhen no WebSocket binding is present in the channel.
33-38: Effective test for WebSocket binding without query parameters.The test properly simulates a WebSocket binding that exists but returns an empty object from
value(), ensuring the function handles this edge case correctly.
40-45: Good coverage for empty query properties.This test case validates the behavior when a query object exists but has no properties, which is an important edge case.
47-66: Comprehensive test for parameter extraction.This test excellently covers multiple scenarios:
- Parameters with string defaults (foo: 'bar')
- Parameters without defaults (baz: '')
- Numeric defaults converted to strings (num: '123')
The assertions correctly verify the expected behavior for each case.
68-71: Simple and effective test for empty channels.Correctly tests the edge case of an empty channels map.
73-83: Excellent edge case coverage.These tests effectively cover scenarios where:
- Channel bindings are undefined (lines 73-77)
- Channel lacks the expected
bindingsfunction (lines 79-83)Both are important defensive programming test cases.
85-96: Great test for malformed data handling.This test validates that the function gracefully handles cases where
propertiesis not an object, which is an excellent defensive programming practice.
26-97: Comprehensive test suite with excellent coverage.The test suite thoroughly covers:
- ✅ Null/missing WebSocket bindings
- ✅ Empty query parameters
- ✅ Parameter extraction with various data types
- ✅ Default value handling
- ✅ Edge cases (empty maps, undefined bindings, malformed data)
- ✅ Error conditions
This provides robust validation of the
getQueryParamsfunction's behavior across all expected scenarios.
achaljhawar
changed the title
achaljhawar
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/helpers/test/bindings.test.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/helpers/test/bindings.test.js (1)
packages/helpers/src/bindings.js (2)
bindings(21-21)wsBinding(28-28)
🪛 GitHub Check: Test NodeJS PR - ubuntu-latest
packages/helpers/test/bindings.test.js
[failure] 31-31:
Define a constant instead of duplicating this literal 7 times
🪛 GitHub Actions: PR testing - if Node project
packages/helpers/test/bindings.test.js
[error] 31-31: ESLint: Define a constant instead of duplicating this literal 7 times (sonarjs/no-duplicate-string)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Acceptance tests for generated templates
🔇 Additional comments (10)
packages/helpers/test/bindings.test.js (10)
1-4: Good test setup with appropriate imports and constants.The imports and WS_BINDING_KEY constant are well-defined and follow good testing practices.
5-16: Well-designed MockChannel class for testing.The MockChannel class properly simulates the channel interface with a realistic bindings() method implementation. The mock correctly handles the Map-like interface with has() and get() methods.
18-26: MockChannelsMap extends Map appropriately.The class extends Map and adds the required isEmpty() and all() methods that match the expected interface for the channels parameter.
35-40: Test correctly validates empty WebSocket binding.The test properly checks that when a WebSocket binding exists but has no query parameters, the function returns null.
42-47: Good edge case coverage for empty query properties.This test ensures the function handles the case where query exists but has no properties correctly.
49-68: Comprehensive test for query parameter extraction and type conversion.This test excellently covers:
- Parameters with default values
- Parameters without default values (empty string)
- Numeric default values converted to strings
- Proper Map interface usage for the returned parameters
The assertions verify all expected behaviors of the function.
70-73: Appropriate test for empty channels map.This edge case test ensures the function handles empty input gracefully.
75-79: Good test for undefined bindings.This test covers the case where the bindings() method returns undefined, which is a realistic edge case.
81-85: Important test for missing bindings function.This test ensures robustness when channel objects don't have the expected bindings() method.
87-98: Excellent edge case for malformed properties.This test ensures the function handles malformed data gracefully when properties is not an object as expected.
|
hey @achaljhawar The tests are failing. The linter tests are failing. Run |
|
@ItshMoh I have fixed the linting error |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/helpers/test/bindings.test.js (1)
1-100: Consider adding test for multiple channels scenario.The test suite is comprehensive and well-structured. Consider adding a test case to verify behavior when multiple channels exist, ensuring the function correctly finds and processes the first WebSocket binding.
+ it('should return query params from first channel with WebSocket binding when multiple channels exist', () => { + const channels = new MockChannelsMap(); + const wsBinding = { + value: () => ({ + query: { + properties: { + testParam: { default: 'firstChannel' }, + }, + }, + }), + }; + + channels.set('channel1', new MockChannel(null)); // No WS binding + channels.set('channel2', new MockChannel(wsBinding)); // Has WS binding + channels.set('channel3', new MockChannel({ value: () => ({ query: { properties: { other: { default: 'second' } } } }) })); // Another WS binding + + const params = getQueryParams(channels); + expect(params).not.toBeNull(); + expect(params.get('testParam')).toBe('firstChannel'); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/helpers/test/bindings.test.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Acceptance tests for generated templates
🔇 Additional comments (11)
packages/helpers/test/bindings.test.js (11)
3-4: LGTM! Constants properly defined.Good use of constants to avoid string duplication, addressing the previous review feedback effectively.
6-17: Well-designed mock implementation.The
MockChannelclass effectively simulates the expected channel behavior with properbindings()method that returns a Map-like interface. The implementation correctly handles the WebSocket binding key check and returns appropriate values.
19-27: Appropriate mock extending Map functionality.The
MockChannelsMapextends the nativeMapwith the requiredisEmpty()andall()methods, providing a clean simulation of the actual channels map interface.
30-34: Good edge case coverage.This test correctly verifies the null return when no WebSocket binding exists, which is an important edge case.
36-41: Validates missing query parameters scenario.Properly tests the case where WebSocket binding exists but lacks query parameters, ensuring robust error handling.
43-48: Tests empty properties scenario effectively.This test case ensures the function handles WebSocket bindings with query objects that lack properties, which is another important edge case.
50-69: Comprehensive testing of successful parameter extraction.This test effectively validates:
- Parameters with default values are correctly extracted
- Parameters without defaults get empty string values
- Numeric defaults are converted to strings (line 68)
- The function returns a proper Map-like object
The test covers the main happy path scenario thoroughly.
71-74: Good coverage of empty channels scenario.Tests the edge case of an empty channels map, ensuring the function handles this gracefully.
76-80: Tests undefined bindings scenario.This test ensures the function handles cases where the channel's
bindings()method returnsundefined, which is good defensive programming validation.
82-86: Validates malformed channel objects.Tests the scenario where a channel object doesn't have the expected
bindingsfunction, ensuring the function doesn't crash on unexpected input.
88-99: Excellent edge case testing.This test validates that the function gracefully handles malformed query data where
propertiesis not an object, preventing potential runtime errors.
derberg
changed the title
There was a problem hiding this comment.
please refactor test and follow approach from other helper tests where we do not mock data inside test but reuse a test fixture like this https://github.com/asyncapi/generator/blob/master/packages/helpers/test/servers.test.js#L6
|
@derberg I have updated bindings.test.js to use test fixtures |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/helpers/test/__fixtures__/asyncapi-websocket-query.yml (1)
146-146: Add a newline at end of file
YAML files should end with a trailing newline to satisfy linters and avoid parsing warnings.--- a/packages/helpers/test/__fixtures__/asyncapi-websocket-query.yml +++ b/packages/helpers/test/__fixtures__/asyncapi-websocket-query.yml @@ -146 +147 - ethusd +🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 146-146: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/helpers/test/__fixtures__/asyncapi-websocket-query.yml(2 hunks)packages/helpers/test/bindings.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/helpers/test/bindings.test.js
🧰 Additional context used
🪛 YAMLlint (1.37.1)
packages/helpers/test/__fixtures__/asyncapi-websocket-query.yml
[error] 146-146: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test generator as dependency with Node 18
- GitHub Check: Test generator as dependency with Node 20
🔇 Additional comments (2)
packages/helpers/test/__fixtures__/asyncapi-websocket-query.yml (2)
77-81: Correctly omitqueryfor the missing-query fixture
ThewsBindingNoQuerychannel intentionally leaves out thequerysection to validate behavior when no query parameters are defined. Indentation andbindingVersionalign with the other fixtures.
82-88: Correct implementation of an empty query schema fixture
ThewsBindingEmptyQuerychannel defines an emptyqueryobject (type: object), which is exactly what you need to test empty-query schema handling. Structure and indentation are consistent.
| const filteredChannels = new Map(); | ||
| const channelWithoutBinding = channels.get('marketDataV1NoBinding'); | ||
| if (channelWithoutBinding) { | ||
| filteredChannels.set('marketDataV1NoBinding', channelWithoutBinding); | ||
| } | ||
|
|
||
| filteredChannels.isEmpty = function() { return this.size === 0; }; | ||
| filteredChannels.all = function() { return this; }; |
There was a problem hiding this comment.
instead of doing this, and need for mocking
how about just using delete on the map? deleting channels that are not needed for given test case?
There was a problem hiding this comment.
hey, I see you did some updates but you did not answer the question
There was a problem hiding this comment.
instead of doing this, and need for mocking
how about just using
deleteon the map? deleting channels that are not needed for given test case?
Yeah, this can be done
There was a problem hiding this comment.
can you check my latest commit I used Map.delete() to implement the fix
There was a problem hiding this comment.
I have changed the variable names accordingly
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/helpers/test/__fixtures__/asyncapi-websocket-query.yml (2)
83-89: Explicitly define emptypropertiesfor empty query schema
While definingquery: { type: object }is valid, adding an explicitproperties: {}field makes the intent clearer and aligns with other fixtures that declareproperties. This can help avoid ambiguity when the schema is consumed programmatically.Suggested diff:
wsBindingEmptyQuery: address: '/empty-query' bindings: ws: bindingVersion: 0.1.0 - query: - type: object + query: + type: object + properties: {}
162-162: Add a newline at end of file
YAML files should end with a single newline character to satisfy lint rules (new-line-at-end-of-file). Please add one.--- a/packages/helpers/test/__fixtures__/asyncapi-websocket-query.yml +++ b/packages/helpers/test/__fixtures__/asyncapi-websocket-query.yml @@ components: - - ethusd + - ethusd +🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 162-162: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/helpers/test/__fixtures__/asyncapi-websocket-query.yml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
packages/helpers/test/__fixtures__/asyncapi-websocket-query.yml
[error] 162-162: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test generator as dependency with Node 20
- GitHub Check: Test generator as dependency with Node 18
🔇 Additional comments (1)
packages/helpers/test/__fixtures__/asyncapi-websocket-query.yml (1)
77-82: Correct handling of channels withoutquerybinding
Great addition of thewsBindingNoQueryfixture to cover the case wherebindings.ws.queryis undefined. This will ensuregetQueryParamscorrectly falls back (e.g., to an empty object) when no query schema is provided.
|
@derberg I have implemented the changes you asked me to make |
| const parser = new Parser(); | ||
| const asyncapi_v3_path = path.resolve(__dirname, './__fixtures__/asyncapi-websocket-query.yml'); | ||
|
|
||
| function createChannelsWithOnly(keysToKeep, originalChannels) { |
There was a problem hiding this comment.
why we need to create a new map here, and still do some mocking basically of all and isEmpty
There was a problem hiding this comment.
hey are you still working on something?
There was a problem hiding this comment.
why we need to create a new map here, and still do some mocking basically of all and isEmpty
There was a problem hiding this comment.
ohk I'll fix this
There was a problem hiding this comment.
should I create a second fixture file to fix this problem?
or should I test with the full channel object and verify its behaviour?
There was a problem hiding this comment.
let's not decide yet, first answer my question 😄 why new map is needed, why you cannot operate in the helper on the map you get as input
There was a problem hiding this comment.
The new Map is needed to create a copy so the original channels map isn't mutated, which would affect other tests that need the data.
There was a problem hiding this comment.
oh, now much better. Remember reviewer did not code and do not see what issues you had during development. Sometimes we just ask question not to play smart, but to really get an answer 😄 thanks for the context - now it makes sense why you've done so, and why later you still do mocking of all and isEmpty - please add comment to tested code to explain why this mocking is needed
|
you pinged me for review again, but I see no changes, and no answers to questions |
|
@derberg sorry I pinged you before , I made some changes that remove mocking and refactored the |
| const parser = new Parser(); | ||
| const asyncapi_v3_path = path.resolve(__dirname, './__fixtures__/asyncapi-websocket-query.yml'); | ||
|
|
||
| function createChannelsWithOnly(keysToKeep, originalChannels) { |
There was a problem hiding this comment.
oh, now much better. Remember reviewer did not code and do not see what issues you had during development. Sometimes we just ask question not to play smart, but to really get an answer 😄 thanks for the context - now it makes sense why you've done so, and why later you still do mocking of all and isEmpty - please add comment to tested code to explain why this mocking is needed
|
|
@derberg I have made some changes according to your suggestions |
Description
packages/helpers/src/bindings.js.Related issue(s)
resolves #1538
Summary by CodeRabbit
Summary by CodeRabbit