Skip to content
Merged
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
198 changes: 198 additions & 0 deletions app/client/src/widgets/JSONFormWidget/component/Form.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,198 @@
import "@testing-library/jest-dom";
import { fireEvent, render, screen } from "@testing-library/react";
import { klona } from "klona";
import React from "react";
import { useForm } from "react-hook-form";
import { ROOT_SCHEMA_KEY, type SchemaItem } from "../constants";
import Form from "./Form";
import useFixedFooter from "./useFixedFooter";

jest.mock("react-hook-form", () => ({
__esModule: true,
useForm: jest.fn(),
FormProvider: jest.fn(({ children }) => <div>{children}</div>),
}));

jest.mock("./useFixedFooter", () => ({
__esModule: true,
default: jest.fn(),
}));

jest.mock("widgets/ButtonWidget/component", () => ({
__esModule: true,
BaseButton: jest.fn(({ children, ...props }) => (
<button {...props}>{children}</button>
)),
}));
Comment on lines +10 to +26

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider consolidating mock implementations.

The mock setup for multiple components could be moved to a separate test utils file for better reusability across test suites.

+ // Move to __mocks__/testUtils.ts
+ export const mockComponents = () => {
+   jest.mock("react-hook-form", () => ({
+     __esModule: true,
+     useForm: jest.fn(),
+     FormProvider: jest.fn(({ children }) => <div>{children}</div>),
+   }));
+   
+   // ... other mocks
+ };

Committable suggestion was skipped due to low confidence.


function mockUseForm(withErrors = false) {
(useForm as jest.Mock).mockReturnValue({
formState: {
errors: withErrors
? {
fieldName: {
type: "required",
message: "This field is required",
},
}
: null,
},
reset: jest.fn(),
trigger: jest.fn(),
watch: jest.fn(() => ({
subscribe: jest.fn(),
unsubscribe: jest.fn(),
})),
});
}
Comment on lines +28 to +47

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance mockUseForm to support configurable trigger behavior.

The mockUseForm function should allow configuring the trigger function's return value to properly test validation scenarios.

 function mockUseForm(withErrors = false) {
+  const mockTrigger = jest.fn().mockResolvedValue(!withErrors);
   (useForm as jest.Mock).mockReturnValue({
     formState: {
       errors: withErrors
         ? {
             fieldName: {
               type: "required",
               message: "This field is required",
             },
           }
         : null,
     },
     reset: jest.fn(),
-    trigger: jest.fn(),
+    trigger: mockTrigger,
     watch: jest.fn(() => ({
       subscribe: jest.fn(),
       unsubscribe: jest.fn(),
     })),
   });
 }
📝 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
function mockUseForm(withErrors = false) {
(useForm as jest.Mock).mockReturnValue({
formState: {
errors: withErrors
? {
fieldName: {
type: "required",
message: "This field is required",
},
}
: null,
},
reset: jest.fn(),
trigger: jest.fn(),
watch: jest.fn(() => ({
subscribe: jest.fn(),
unsubscribe: jest.fn(),
})),
});
}
function mockUseForm(withErrors = false) {
const mockTrigger = jest.fn().mockResolvedValue(!withErrors);
(useForm as jest.Mock).mockReturnValue({
formState: {
errors: withErrors
? {
fieldName: {
type: "required",
message: "This field is required",
},
}
: null,
},
reset: jest.fn(),
trigger: mockTrigger,
watch: jest.fn(() => ({
subscribe: jest.fn(),
unsubscribe: jest.fn(),
})),
});
}


const TEST_IDS = {
SUBMIT_BTN: "t--jsonform-submit-btn",
RESET_BTN: "t--jsonform-reset-btn",
};

const defaultProps = {
backgroundColor: "#fff",
disabledWhenInvalid: false,
fixedFooter: false,
getFormData: jest.fn(),
hideFooter: false,
isSubmitting: false,
isWidgetMounting: false,
onFormValidityUpdate: jest.fn(),
onSubmit: jest.fn(),
onreset: jest.fn(),
registerResetObserver: jest.fn(),
resetButtonLabel: "Reset",
resetButtonStyles: {},
scrollContents: false,
showReset: true,
stretchBodyVertically: false,
submitButtonLabel: "Submit",
submitButtonStyles: {},
title: "Test Form",
unregisterResetObserver: jest.fn(),
updateFormData: jest.fn(),
children: <input />,
};

describe("Form Component", () => {
beforeEach(() => {
const mockBodyRef = React.createRef<HTMLFormElement>();
const mockFooterRef = React.createRef<HTMLDivElement>();

(useFixedFooter as jest.Mock).mockReturnValue({
bodyRef: mockBodyRef,
footerRef: mockFooterRef,
});
});

describe("happy render path", () => {
beforeEach(() => mockUseForm());
it("renders the form title", () => {
render(<Form {...defaultProps}>Form Content</Form>);
expect(screen.getByText("Test Form")).toBeInTheDocument();
});

it("renders children inside the form body", () => {
render(<Form {...defaultProps}>Form Content</Form>);
expect(screen.getByText("Form Content")).toBeInTheDocument();
});

it("renders the submit button with correct label", () => {
const { getByTestId } = render(
<Form {...defaultProps}>Form Content</Form>,
);
const submitBtn = getByTestId(TEST_IDS.SUBMIT_BTN);

expect(submitBtn).toBeInTheDocument();
expect(submitBtn).toHaveAttribute("text", defaultProps.submitButtonLabel);

fireEvent.click(submitBtn);
expect(defaultProps.onSubmit).toHaveBeenCalled();
});

it("renders the reset button with correct label when showReset is true", () => {
const { getByTestId } = render(
<Form {...defaultProps}>Form Content</Form>,
);
const resetBtn = getByTestId(TEST_IDS.RESET_BTN);

expect(resetBtn).toBeInTheDocument();
expect(resetBtn).toHaveAttribute("text", defaultProps.resetButtonLabel);
expect(defaultProps.registerResetObserver).toHaveBeenCalled();
});

// Form updates data correctly when input values change
it("should update data when input values change", () => {
const mockUpdateFormData = jest.fn();
const mockGetFormData = jest.fn().mockReturnValue({});
const mockRegisterResetObserver = jest.fn();
const mockUnregisterResetObserver = jest.fn();
const mockOnSubmit = jest.fn();
const mockOnFormValidityUpdate = jest.fn();
const mockSchema = { [ROOT_SCHEMA_KEY]: {} as SchemaItem };
const props = klona({
...defaultProps,
updateFormData: mockUpdateFormData,
getFormData: mockGetFormData,
registerResetObserver: mockRegisterResetObserver,
unregisterResetObserver: mockUnregisterResetObserver,
onSubmit: mockOnSubmit,
onFormValidityUpdate: mockOnFormValidityUpdate,
schema: mockSchema,
});
const { getByTestId } = render(<Form {...props} />);

fireEvent.click(getByTestId(TEST_IDS.SUBMIT_BTN));
expect(mockUpdateFormData).toHaveBeenCalled();
});
Comment on lines +127 to +149

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Test implementation doesn't match its description.

The test claims to verify input value changes but only tests form submission. Either rename the test or implement proper input change verification.

-it("should update data when input values change", () => {
+it("should update form data on submit", () => {
   const mockUpdateFormData = jest.fn();
   const mockGetFormData = jest.fn().mockReturnValue({});
   const mockRegisterResetObserver = jest.fn();
   const mockUnregisterResetObserver = jest.fn();
   const mockOnSubmit = jest.fn();
   const mockOnFormValidityUpdate = jest.fn();
   const mockSchema = { [ROOT_SCHEMA_KEY]: {} as SchemaItem };
   const props = klona({
     ...defaultProps,
     updateFormData: mockUpdateFormData,
     getFormData: mockGetFormData,
     registerResetObserver: mockRegisterResetObserver,
     unregisterResetObserver: mockUnregisterResetObserver,
     onSubmit: mockOnSubmit,
     onFormValidityUpdate: mockOnFormValidityUpdate,
     schema: mockSchema,
   });
   const { getByTestId } = render(<Form {...props} />);

   fireEvent.click(getByTestId(TEST_IDS.SUBMIT_BTN));
   expect(mockUpdateFormData).toHaveBeenCalled();
 });
📝 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
it("should update data when input values change", () => {
const mockUpdateFormData = jest.fn();
const mockGetFormData = jest.fn().mockReturnValue({});
const mockRegisterResetObserver = jest.fn();
const mockUnregisterResetObserver = jest.fn();
const mockOnSubmit = jest.fn();
const mockOnFormValidityUpdate = jest.fn();
const mockSchema = { [ROOT_SCHEMA_KEY]: {} as SchemaItem };
const props = klona({
...defaultProps,
updateFormData: mockUpdateFormData,
getFormData: mockGetFormData,
registerResetObserver: mockRegisterResetObserver,
unregisterResetObserver: mockUnregisterResetObserver,
onSubmit: mockOnSubmit,
onFormValidityUpdate: mockOnFormValidityUpdate,
schema: mockSchema,
});
const { getByTestId } = render(<Form {...props} />);
fireEvent.click(getByTestId(TEST_IDS.SUBMIT_BTN));
expect(mockUpdateFormData).toHaveBeenCalled();
});
it("should update form data on submit", () => {
const mockUpdateFormData = jest.fn();
const mockGetFormData = jest.fn().mockReturnValue({});
const mockRegisterResetObserver = jest.fn();
const mockUnregisterResetObserver = jest.fn();
const mockOnSubmit = jest.fn();
const mockOnFormValidityUpdate = jest.fn();
const mockSchema = { [ROOT_SCHEMA_KEY]: {} as SchemaItem };
const props = klona({
...defaultProps,
updateFormData: mockUpdateFormData,
getFormData: mockGetFormData,
registerResetObserver: mockRegisterResetObserver,
unregisterResetObserver: mockUnregisterResetObserver,
onSubmit: mockOnSubmit,
onFormValidityUpdate: mockOnFormValidityUpdate,
schema: mockSchema,
});
const { getByTestId } = render(<Form {...props} />);
fireEvent.click(getByTestId(TEST_IDS.SUBMIT_BTN));
expect(mockUpdateFormData).toHaveBeenCalled();
});

});
Comment on lines +90 to +150

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add missing test coverage for form validation triggers.

The "happy render path" suite is missing tests for form validation triggers on child component updates.

Add the following test:

it("should trigger validation on child component updates", async () => {
  const mockTrigger = jest.fn().mockResolvedValue(true);
  (useForm as jest.Mock).mockReturnValue({
    ...defaultMockFormValues,
    trigger: mockTrigger,
    formState: { errors: {} },
  });

  const { rerender } = render(
    <Form {...defaultProps}>
      <input value="initial" />
    </Form>
  );

  rerender(
    <Form {...defaultProps}>
      <input value="updated" />
    </Form>
  );

  expect(mockTrigger).toHaveBeenCalled();
});


describe("unhappy render path", () => {
it("does not render the reset button when showReset is false", () => {
mockUseForm();
const { queryByTestId } = render(
<Form {...defaultProps} showReset={false}>
Form Content
</Form>,
);

expect(queryByTestId(TEST_IDS.RESET_BTN)).not.toBeInTheDocument();
});

it("disables the submit button when disabledWhenInvalid is true and form is invalid", () => {
mockUseForm(true);
const { getByTestId } = render(
<Form {...defaultProps} disabledWhenInvalid>
Form Content
</Form>,
);

expect(getByTestId(TEST_IDS.SUBMIT_BTN)).toBeDisabled();
});

it("triggers a check, necessitating a re-render, when the children in form are updated", () => {
mockUseForm(true);
const propsToUpdate = klona({
...defaultProps,
disabledWhenInvalid: true,
});
const { getByTestId } = render(
<Form {...propsToUpdate}>Form Content</Form>,
);

expect(getByTestId(TEST_IDS.SUBMIT_BTN)).toBeDisabled();
});
Comment on lines +175 to +186

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve test assertions for child component updates.

The test lacks proper assertions to verify the trigger behavior when children are updated.

-it("triggers a check, necessitating a re-render, when the children in form are updated", () => {
+it("should trigger validation when children are updated", () => {
   mockUseForm(true);
   const propsToUpdate = klona({
     ...defaultProps,
     disabledWhenInvalid: true,
   });
-  const { getByTestId } = render(
+  const { getByTestId, rerender } = render(
     <Form {...propsToUpdate}>Form Content</Form>,
   );
+  
+  rerender(<Form {...propsToUpdate}>Updated Content</Form>);
+  
+  const mockTrigger = (useForm as jest.Mock).mock.results[0].value.trigger;
+  expect(mockTrigger).toHaveBeenCalled();
   expect(getByTestId(TEST_IDS.SUBMIT_BTN)).toBeDisabled();
 });
📝 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
it("triggers a check, necessitating a re-render, when the children in form are updated", () => {
mockUseForm(true);
const propsToUpdate = klona({
...defaultProps,
disabledWhenInvalid: true,
});
const { getByTestId } = render(
<Form {...propsToUpdate}>Form Content</Form>,
);
expect(getByTestId(TEST_IDS.SUBMIT_BTN)).toBeDisabled();
});
it("should trigger validation when children are updated", () => {
mockUseForm(true);
const propsToUpdate = klona({
...defaultProps,
disabledWhenInvalid: true,
});
const { getByTestId, rerender } = render(
<Form {...propsToUpdate}>Form Content</Form>,
);
rerender(<Form {...propsToUpdate}>Updated Content</Form>);
const mockTrigger = (useForm as jest.Mock).mock.results[0].value.trigger;
expect(mockTrigger).toHaveBeenCalled();
expect(getByTestId(TEST_IDS.SUBMIT_BTN)).toBeDisabled();
});


it("should handle empty schema gracefully without errors", () => {
mockUseForm();
const mockUpdateFormData = jest.fn();
const props = klona({ ...defaultProps, schema: undefined });
const { container } = render(<Form {...props} />);

expect(container).toBeInTheDocument();
expect(mockUpdateFormData).not.toHaveBeenCalled();
});
});
});
2 changes: 2 additions & 0 deletions app/client/src/widgets/JSONFormWidget/component/Form.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ function Form<TValues = any>(
<Button
{...resetButtonStyles}
className="t--jsonform-reset-btn"
data-testid="t--jsonform-reset-btn"
onClick={(e) => onReset(schema, e)}
text={resetButtonLabel}
type="reset"
Expand All @@ -308,6 +309,7 @@ function Form<TValues = any>(
<Button
{...submitButtonStyles}
className="t--jsonform-submit-btn"
data-testid="t--jsonform-submit-btn"
disabled={disabledWhenInvalid && isFormInValid}
loading={isSubmitting}
onClick={onSubmit}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
import type { InputHTMLType } from "widgets/BaseInputWidget/component";
import BaseInputComponent from "widgets/BaseInputWidget/component";
import { BASE_LABEL_TEXT_SIZE } from "../component/FieldLabel";
import useUnmountFieldValidation from "./useUnmountFieldValidation";

export type BaseInputComponentProps = FieldComponentBaseProps &
FieldEventProps & {
Expand Down Expand Up @@ -240,6 +241,7 @@ function BaseInputField<TSchemaItem extends SchemaItem>({
fieldType: schemaItem.fieldType,
isValid: isValueValid,
});
useUnmountFieldValidation({ fieldName: name });

const { inputRef } = useEvents<HTMLInputElement | HTMLTextAreaElement>({
fieldBlurHandler: onBlur,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { EventType } from "constants/AppsmithActionConstants/ActionConstants";
import { Colors } from "constants/Colors";
import { BASE_LABEL_TEXT_SIZE } from "../component/FieldLabel";
import { LabelPosition } from "components/constants";
import useUnmountFieldValidation from "./useUnmountFieldValidation";

type CheckboxComponentProps = FieldComponentBaseProps &
FieldEventProps & {
Expand Down Expand Up @@ -82,6 +83,7 @@ function CheckboxField({
fieldType: schemaItem.fieldType,
isValid: isValueValid,
});
useUnmountFieldValidation({ fieldName: name });

const onCheckChange = useCallback(
(isChecked: boolean) => {
Expand Down
2 changes: 2 additions & 0 deletions app/client/src/widgets/JSONFormWidget/fields/DateField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { ISO_DATE_FORMAT } from "constants/WidgetValidation";
import { TimePrecision } from "widgets/DatePickerWidget2/constants";
import { Colors } from "constants/Colors";
import { BASE_LABEL_TEXT_SIZE } from "../component/FieldLabel";
import useUnmountFieldValidation from "./useUnmountFieldValidation";

type DateComponentProps = FieldComponentBaseProps &
FieldEventProps & {
Expand Down Expand Up @@ -133,6 +134,7 @@ function DateField({
fieldName: name,
fieldType,
});
useUnmountFieldValidation({ fieldName: name });

const onDateSelected = useCallback(
(selectedValue: string) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { EventType } from "constants/AppsmithActionConstants/ActionConstants";
import { isPrimitive, validateOptions } from "../helper";
import { Colors } from "constants/Colors";
import { BASE_LABEL_TEXT_SIZE } from "../component/FieldLabel";
import useUnmountFieldValidation from "./useUnmountFieldValidation";

type MultiSelectComponentProps = FieldComponentBaseProps &
FieldEventProps & {
Expand Down Expand Up @@ -129,6 +130,7 @@ function MultiSelectField({
fieldName: name,
fieldType,
});
useUnmountFieldValidation({ fieldName: name });

const [updateFilterText] = useUpdateInternalMetaState({
propertyName: `${name}.filterText`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { ActionUpdateDependency } from "../constants";
import { EventType } from "constants/AppsmithActionConstants/ActionConstants";
import { Colors } from "constants/Colors";
import { BASE_LABEL_TEXT_SIZE } from "../component/FieldLabel";
import useUnmountFieldValidation from "./useUnmountFieldValidation";

type RadioGroupComponentProps = FieldComponentBaseProps & {
options: RadioOption[];
Expand Down Expand Up @@ -66,6 +67,7 @@ function RadioGroupField({
fieldName: name,
fieldType: schemaItem.fieldType,
});
useUnmountFieldValidation({ fieldName: name });

const onSelectionChange = useCallback(
(selectedValue: string) => {
Expand Down
2 changes: 2 additions & 0 deletions app/client/src/widgets/JSONFormWidget/fields/SelectField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { isPrimitive } from "../helper";
import { isNil } from "lodash";
import { Colors } from "constants/Colors";
import { BASE_LABEL_TEXT_SIZE } from "../component/FieldLabel";
import useUnmountFieldValidation from "./useUnmountFieldValidation";

interface MetaProps {
filterText?: string;
Expand Down Expand Up @@ -106,6 +107,7 @@ function SelectField({
fieldName: name,
fieldType: schemaItem.fieldType,
});
useUnmountFieldValidation({ fieldName: name });

const [updateFilterText] = useUpdateInternalMetaState({
propertyName: `${name}.filterText`,
Expand Down
2 changes: 2 additions & 0 deletions app/client/src/widgets/JSONFormWidget/fields/SwitchField.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { EventType } from "constants/AppsmithActionConstants/ActionConstants";
import { Colors } from "constants/Colors";
import { BASE_LABEL_TEXT_SIZE } from "../component/FieldLabel";
import { LabelPosition } from "components/constants";
import useUnmountFieldValidation from "./useUnmountFieldValidation";

type SwitchComponentOwnProps = FieldComponentBaseProps &
FieldEventProps & {
Expand Down Expand Up @@ -70,6 +71,7 @@ function SwitchField({
fieldType: schemaItem.fieldType,
isValid: isValueValid,
});
useUnmountFieldValidation({ fieldName: name });

const onSwitchChange = useCallback(
(value: boolean) => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import { renderHook } from "@testing-library/react-hooks";
import { useFormContext } from "react-hook-form";
import useUnmountFieldValidation from "./useUnmountFieldValidation";
import { startAndEndSpanForFn } from "UITelemetry/generateTraces";

// Mock dependencies
jest.mock("react-hook-form", () => ({
useFormContext: jest.fn(),
}));

jest.mock("UITelemetry/generateTraces", () => ({
startAndEndSpanForFn: jest.fn((name, options, fn) => fn()),
}));

describe("useUnmountFieldValidation", () => {
const mockTrigger = jest.fn();

beforeEach(() => {
(useFormContext as jest.Mock).mockReturnValue({
trigger: mockTrigger,
});
});

afterEach(() => {
jest.clearAllMocks();
});

it("should trigger validation on unmount", () => {
const fieldName = "testField";
const { unmount } = renderHook(() =>
useUnmountFieldValidation({ fieldName }),
);

// Verify trigger hasn't been called yet
expect(mockTrigger).not.toHaveBeenCalled();
expect(startAndEndSpanForFn).not.toHaveBeenCalled();

// Unmount the hook
unmount();

// Verify trigger was called with correct field name
expect(startAndEndSpanForFn).toHaveBeenCalledWith(
"JSONFormWidget.triggerFieldValidation",
{},
expect.any(Function),
);
expect(mockTrigger).toHaveBeenCalledWith(fieldName);
});

it("should update cleanup when fieldName changes", () => {
const { rerender, unmount } = renderHook(
({ fieldName }) => useUnmountFieldValidation({ fieldName }),
{
initialProps: { fieldName: "field1" },
},
);

// Change the field name
rerender({ fieldName: "field2" });
unmount();

// Should trigger validation for the latest field name
expect(mockTrigger).toHaveBeenCalledWith("field2");
});
});
Loading