Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 84 additions & 4 deletions app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.test.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from "react";
import { render } from "@testing-library/react";
import { render, screen } from "@testing-library/react";
import configureStore from "redux-mock-store";
import { Provider } from "react-redux";
import { ThemeProvider } from "styled-components";
Expand All @@ -10,9 +10,36 @@ import { EditorViewMode } from "ee/entities/IDE/constants";
import "@testing-library/jest-dom/extend-expect";
import QueryDebuggerTabs from "./QueryDebuggerTabs";
import { ENTITY_TYPE } from "ee/entities/AppsmithConsole/utils";
import type { ActionResponse } from "api/ActionAPI";

const mockStore = configureStore([]);

const mockSuccessResponse: ActionResponse = {
body: ["Record 1", "Record 2"],
statusCode: "200",
dataTypes: [],
duration: "3000",
size: "200",
isExecutionSuccess: true,
headers: {
"Content-Type": ["application/json"],
"Cache-Control": ["no-cache"],
},
};

const mockFailedResponse: ActionResponse = {
body: [{ response: "Failed" }],
statusCode: "200",
dataTypes: [],
duration: "3000",
size: "200",
isExecutionSuccess: false,
headers: {
"Content-Type": ["application/json"],
"Cache-Control": ["no-cache"],
},
};

const storeState = {
...unitTestBaseMockStore,
evaluations: {
Expand Down Expand Up @@ -54,9 +81,7 @@ const storeState = {
};

describe("ApiResponseView", () => {
// TODO: Fix this the next time the file is edited
// eslint-disable-next-line @typescript-eslint/no-explicit-any
let store: any;
let store = mockStore(storeState);

beforeEach(() => {
store = mockStore(storeState);
Expand Down Expand Up @@ -88,4 +113,59 @@ describe("ApiResponseView", () => {
?.classList.contains("select-text"),
).toBe(true);
});
it("should show record count as result if the query response returns records", () => {
render(
<Provider store={store}>
<ThemeProvider theme={lightTheme}>
<Router>
<QueryDebuggerTabs
actionName="Query1"
actionResponse={mockSuccessResponse}
actionSource={{
id: "ID1",
name: "Query1",
type: ENTITY_TYPE.ACTION,
}}
isRunning={false}
onRunClick={() => {}}
/>
</Router>
</ThemeProvider>
</Provider>,
);

const expectedResultText = "Result: 2 Records";
const resultTextElement = screen.getByTestId("result-text");

expect(resultTextElement).toBeInTheDocument();
expect(resultTextElement?.textContent).toContain(expectedResultText);
});

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);
});
Comment on lines +144 to +170
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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>
  );
};

});
16 changes: 12 additions & 4 deletions app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ const ResultsCount = styled.div`
color: var(--ads-v2-color-fg);
`;

const ErrorText = styled(Text)`
color: var(--ads-v2-colors-action-error-label-default-fg);
`;

interface QueryDebuggerTabsProps {
actionSource: SourceEntity;
currentActionConfig?: Action;
Expand Down Expand Up @@ -274,11 +278,15 @@ function QueryDebuggerTabs({
>
{output && !!output.length && (
<ResultsCount>
<Text type={TextType.P3}>
<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>
)}
Comment on lines +281 to +289
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Removed nested Text component for simpler structure
  2. Simplified string interpolation
  3. Added accessibility attributes
  4. 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.

Suggested change
<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>

</Text>
</ResultsCount>
)}
Expand Down