test: add tests and snapshot for QueryParamsVariables component #1622
Conversation
blazethunderstorm
requested review from
derberg,
magicmatatjahu,
jonaslagoni and
asyncapi-bot-eve
as code owners
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughA new test suite was introduced for the Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. 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
|
|
@Adi-204 pls review the pr , thanks |
![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/templates/clients/websocket/python/test/components/QueryParamsVariables.test.js (1)
8-11: Consider using a more robust path resolution approach.The current file path construction uses multiple directory traversals (
../../../../../../) which makes it brittle and prone to breaking if the directory structure changes.Consider using a more robust approach:
-const asyncapiFilePath = path.resolve( - __dirname, - '../../../../../../helpers/test/__fixtures__/asyncapi-websocket-query.yml' -); +const asyncapiFilePath = path.resolve( + process.cwd(), + 'helpers/test/__fixtures__/asyncapi-websocket-query.yml' +);Or use a helper function to locate the project root and build paths from there.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/templates/clients/websocket/python/test/components/__snapshots__/QueryParamsVariables.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
packages/templates/clients/websocket/python/test/components/QueryParamsVariables.test.js(1 hunks)
🔇 Additional comments (3)
packages/templates/clients/websocket/python/test/components/QueryParamsVariables.test.js (3)
16-23: Excellent error handling in async setup.The
beforeAllhook properly handles the asynchronous AsyncAPI document parsing with comprehensive error handling. The descriptive error message will help with debugging if the fixture file is missing or invalid.
25-31: Robust test implementation with proper null checking.The test correctly:
- Extracts query parameters from the parsed AsyncAPI document
- Handles potential null values from
getQueryParams- Converts the Map to Array format as expected by the component
- Uses snapshot testing for comprehensive rendering verification
33-41: Comprehensive edge case coverage.Both test cases properly verify the component's behavior when query parameters are
nullorundefined, ensuring the component gracefully handles missing data by returning an empty string.
There was a problem hiding this comment.
@blazethunderstorm also please add 1 more testcase in which directly pass empty array. something like - <QueryParamsVariables queryParams={[]} />
| try { | ||
| const parseResult = await fromFile(parser, asyncapiFilePath).parse(); | ||
| parsedAsyncAPIDocument = parseResult.document; | ||
| } catch (error) { | ||
| throw new Error(`Failed to parse AsyncAPI document: ${error.message}`); | ||
| } |
There was a problem hiding this comment.
remove try and catch from here also write code inside try directly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/templates/clients/websocket/python/test/components/Requires.test.js (1)
16-23: Good error handling, consider adding document validation.The beforeAll setup with error handling is well-implemented. However, consider adding validation to ensure the parsed document contains expected channels/query parameters.
beforeAll(async () => { try { const parseResult = await fromFile(parser, asyncapiFilePath).parse(); parsedAsyncAPIDocument = parseResult.document; + + // Validate that the document has the expected structure + if (!parsedAsyncAPIDocument.channels() || parsedAsyncAPIDocument.channels().length === 0) { + throw new Error('AsyncAPI document does not contain expected channels'); + } } catch (error) { throw new Error(`Failed to parse AsyncAPI document: ${error.message}`); } });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/templates/clients/websocket/python/test/components/__snapshots__/Requires.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
packages/templates/clients/websocket/python/test/components/QueryParamsVariables.test.js(1 hunks)packages/templates/clients/websocket/python/test/components/Requires.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/templates/clients/websocket/python/test/components/QueryParamsVariables.test.js
🔇 Additional comments (2)
packages/templates/clients/websocket/python/test/components/Requires.test.js (2)
1-5: LGTM!The imports are well-organized and include all necessary dependencies for testing the Requires component with AsyncAPI integration.
25-40: Verify fixture file exists and add data validation.The test cases provide good coverage of different scenarios, but consider adding verification that the fixture file exists and contains expected data.
#!/bin/bash # Description: Verify the AsyncAPI fixture file exists and contains expected query parameters # Expected: File should exist and contain channels with query parameters # Check if the fixture file exists fixture_path="packages/helpers/test/__fixtures__/asyncapi-websocket-query.yml" if [ -f "$fixture_path" ]; then echo "✓ Fixture file exists: $fixture_path" # Check if the file contains query-related content if grep -q "query" "$fixture_path"; then echo "✓ Fixture file contains query-related content" # Show relevant sections echo "Query-related content in fixture:" grep -A 5 -B 5 "query" "$fixture_path" else echo "⚠ Fixture file does not contain query-related content" fi else echo "✗ Fixture file not found: $fixture_path" # Search for similar files echo "Searching for similar AsyncAPI fixture files:" fd -e yml -e yaml "asyncapi.*websocket" . fi
| const asyncapiFilePath = path.resolve( | ||
| __dirname, | ||
| '../../../../../../helpers/test/__fixtures__/asyncapi-websocket-query.yml' | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider a more maintainable approach for fixture file path.
The deep relative path with 7 directory levels up (../../../../../../) makes the test fragile to directory structure changes. Consider alternatives for better maintainability.
Options to improve path reliability:
- Use a dedicated test fixtures directory at a consistent location
- Use an environment variable or configuration for the fixtures path
- Copy the fixture to a local test directory
-const asyncapiFilePath = path.resolve(
- __dirname,
- '../../../../../../helpers/test/__fixtures__/asyncapi-websocket-query.yml'
-);
+// Option 1: Use a more stable relative path
+const asyncapiFilePath = path.resolve(__dirname, '../fixtures/asyncapi-websocket-query.yml');
+
+// Option 2: Use process.cwd() for project root
+const asyncapiFilePath = path.resolve(process.cwd(), 'packages/helpers/test/__fixtures__/asyncapi-websocket-query.yml');Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/templates/clients/websocket/python/test/components/Requires.test.js
around lines 8 to 11, the fixture file path uses a fragile deep relative path
with many directory levels up. To improve maintainability, refactor the code to
use a more reliable approach such as placing test fixtures in a dedicated
directory with a consistent path, referencing the fixture path via an
environment variable or configuration setting, or copying the fixture file into
a local test directory within the current test folder. Update the path
resolution accordingly to avoid fragile relative paths.
|
@Adi-204 i have made the eq changes , thanks |
There was a problem hiding this comment.
@blazethunderstorm also we are doing snapshot testing so it is better to replace expect(result).toBe(''); with expect(result.trim()).toMatchSnapshot(); in testcases.
|
@Adi-204 i have made rew changes , thanks |
|
@blazethunderstorm just a suggestion whenever you make changes and feel it is ready for review again. In top of PR on right side you can see Reviewers and it will have my name and a re-request review button, just click on it. |
|
@Adi-204 thanks |
|
@derberg @jonaslagoni @magicmatatjahu pls review it |
|
@blazethunderstorm @Adi-204 tests are failing |
|
@blazethunderstorm test are failing. I think because of snapshots. Run |
|
@Adi-204 i couldn't see any change in snapshots |
|
@blazethunderstorm locally when you are running |
|
@Adi-204 @asyncapi/template-dart-websocket-client#test its showing this error but it was showing for the require.js also but for that tests passed |
|
@blazethunderstorm I think you are talking about the warning about key props. That is just warning not issue.
I have tried this code locally and your snapshots are old run |
|
nope the snapshots arent updated , yeah i am just getting that key prop error |
|
@blazethunderstorm that is strange because I am running same code locally and snapshots are updated. can you please share screenshot of logs(just last part not entire) when you run |
|
|
|
the last part when command is execution is stoped |
|
|
|
@blazethunderstorm thanks, ok their is some kind of problem with dependency. also you can delete and re-install node_modules |
|
@Adi-204 thanks for your help the tests got updated |
|
|
/rtm |
|
@allcontributors please add @blazethunderstorm for test |
|
I've put up a pull request to add @blazethunderstorm! 🎉 |
Description
Added a new test file and corresponding snapshot for the QueryParamsVariables component to verify its rendering behavior with different query parameter inputs. This ensures the component correctly handles query parameters from AsyncAPI documents and renders as expected.
Proof
All test are successfully running
Related issue(s)
Resolves #1545
Summary by CodeRabbit