TESTING EXTERNAL SCRIPT: external merge request from Contributor#37007
TESTING EXTERNAL SCRIPT: external merge request from Contributor#37007AmanAgarwal041 wants to merge 14 commits intoreleasefrom
Conversation
…o fix/query-response-error-count
…o fix/query-response-error-count
…-error-count' into chore/external-contribution-35363
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
|
![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
🧹 Outside diff range and nitpick comments (3)
app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.test.tsx (2)
18-42: LGTM: Mock responses added.The mock responses are well-structured and cover both success and failure scenarios. Consider extracting these mock objects to a separate file for reusability in other tests.
117-143: LGTM: New test case for successful query.The test case effectively checks the record count display. Consider adding an assertion for the exact number of records to ensure precise testing.
app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx (1)
272-282: Improved error handling and testability. Consider extracting the conditional rendering.The changes enhance the component's ability to handle different response scenarios and improve testability. Good job on adding the
data-testidattribute.Consider extracting the conditional rendering into a separate function for better readability:
const renderResultText = () => { if (actionResponse?.isExecutionSuccess) { return ( <Text type={TextType.H5}>{` ${output.length} Record${ output.length > 1 ? "s" : "" }`}</Text> ); } return ( <Text color="red" type={TextType.H5}> {" Error"} </Text> ); };Then use it in the JSX:
<Text data-testid="result-text" type={TextType.P3}> Result: {renderResultText()} </Text>This would make the component more maintainable and easier to test.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.test.tsx (3 hunks)
- app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.test.tsx (3)
2-2: LGTM: New imports added.The new imports are correctly added and necessary for the test enhancements.
Also applies to: 5-5
84-84: Improved type safety for store.Good job updating the store type. This enhances type checking and reduces 'any' usage.
145-171: LGTM: New test case for failed query.The test case effectively checks the error message display for failed queries.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11466642433. |
|
Deploy-Preview-URL: https://ce-37007.dp.appsmith.com |
…o fix/query-response-error-count
…external-contribution-35363
…aushan3737/appsmith into chore/external-contribution-35363
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.test.tsx (4 hunks)
- app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx (2 hunks)
🔇 Additional comments (4)
app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.test.tsx (3)
2-2: LGTM! Well-structured mock responses.The mock responses are comprehensive and properly typed, covering both success and failure scenarios with appropriate fields.
Also applies to: 13-13, 17-41
83-83: Good type safety improvement.Removing the
anytype and directly initializing the store with proper typing enhances code reliability.
115-141: LGTM! Clear and focused test case.The test properly validates the success scenario with appropriate assertions.
app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx (1)
46-48: LGTM: Styled component follows design system.The
ErrorTextcomponent correctly uses the design system color variable for error states.
| it("should show error as result if the query response returns the error", () => { | ||
| render( | ||
| <Provider store={store}> | ||
| <ThemeProvider theme={lightTheme}> | ||
| <Router> | ||
| <QueryDebuggerTabs | ||
| actionName="Query1" | ||
| actionResponse={mockFailedResponse} | ||
| actionSource={{ | ||
| id: "ID1", | ||
| name: "Query1", | ||
| type: ENTITY_TYPE.ACTION, | ||
| }} | ||
| isRunning={false} | ||
| onRunClick={() => {}} | ||
| /> | ||
| </Router> | ||
| </ThemeProvider> | ||
| </Provider>, | ||
| ); | ||
|
|
||
| const expectedResultText = "Result: Error"; | ||
| const resultTextElement = screen.getByTestId("result-text"); | ||
|
|
||
| expect(resultTextElement).toBeInTheDocument(); | ||
| expect(resultTextElement?.textContent).toContain(expectedResultText); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing test isolation.
While the test implementation is correct, consider moving the common render setup to a helper function to reduce duplication and improve maintainability.
const renderQueryDebuggerTabs = (actionResponse?: ActionResponse) => {
return render(
<Provider store={store}>
<ThemeProvider theme={lightTheme}>
<Router>
<QueryDebuggerTabs
actionName="Query1"
actionResponse={actionResponse}
actionSource={{
id: "ID1",
name: "Query1",
type: ENTITY_TYPE.ACTION,
}}
isRunning={false}
onRunClick={() => {}}
/>
</Router>
</ThemeProvider>
</Provider>
);
};| <Text data-testid="result-text" type={TextType.P3}> | ||
| Result: | ||
| <Text type={TextType.H5}>{` ${output.length} Record${ | ||
| output.length > 1 ? "s" : "" | ||
| }`}</Text> | ||
| {actionResponse?.isExecutionSuccess ? ( | ||
| <Text type={TextType.H5}>{` ${output.length} Record${ | ||
| output.length > 1 ? "s" : "" | ||
| }`}</Text> | ||
| ) : ( | ||
| <ErrorText type={TextType.H5}>{" Error"}</ErrorText> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve readability and accessibility of the results display.
The current implementation could be simplified and made more accessible.
Consider this refactor:
- <Text data-testid="result-text" type={TextType.P3}>
- Result:
- {actionResponse?.isExecutionSuccess ? (
- <Text type={TextType.H5}>{` ${output.length} Record${
- output.length > 1 ? "s" : ""
- }`}</Text>
- ) : (
- <ErrorText type={TextType.H5}>{" Error"}</ErrorText>
- )}
- </Text>
+ <Text
+ data-testid="result-text"
+ type={TextType.P3}
+ role="status"
+ aria-live="polite"
+ >
+ {actionResponse?.isExecutionSuccess ? (
+ `Result: ${output.length} Record${output.length !== 1 ? 's' : ''}`
+ ) : (
+ 'Result: Error'
+ )}
+ </Text>Changes:
- Removed nested Text component for simpler structure
- Simplified string interpolation
- Added accessibility attributes
- Improved plural condition check
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Text data-testid="result-text" type={TextType.P3}> | |
| Result: | |
| <Text type={TextType.H5}>{` ${output.length} Record${ | |
| output.length > 1 ? "s" : "" | |
| }`}</Text> | |
| {actionResponse?.isExecutionSuccess ? ( | |
| <Text type={TextType.H5}>{` ${output.length} Record${ | |
| output.length > 1 ? "s" : "" | |
| }`}</Text> | |
| ) : ( | |
| <ErrorText type={TextType.H5}>{" Error"}</ErrorText> | |
| )} | |
| <Text | |
| data-testid="result-text" | |
| type={TextType.P3} | |
| role="status" | |
| aria-live="polite" | |
| > | |
| {actionResponse?.isExecutionSuccess ? ( | |
| `Result: ${output.length} Record${output.length !== 1 ? 's' : ''}` | |
| ) : ( | |
| 'Result: Error' | |
| )} | |
| </Text> |
…external-contribution-35363
…external-contribution-35363
sagar-qa007
added
ok-to-test
…external-contribution-35363
|
Closing as the external contributors PR is already merged. |
3 participants
Description
Fixes #
Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11658966145
Commit: 2cfa618
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/EmbedSettings/EmbedSettings_spec.js
- cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js
List of identified flaky tests.Mon, 04 Nov 2024 08:39:43 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
QueryDebuggerTabscomponent to display record counts or error messages based on API response success.Bug Fixes
QueryDebuggerTabscomponent for successful and failed API responses.Documentation
data-testidattributes for improved testing capabilities.