1222 vitest widgets #1605

marcellmueller · 2024-05-03T23:04:31Z

Implements #1222

Added Vitest coverage report which can be run using yarn coverage. We haven't discussed testing coverage but could be a good topic for dev team agreements. Most of these tests have good coverage but there are a few that could be improved especially the FileWidget.

marcellmueller · 2024-05-03T23:22:40Z

bciers/apps/registration/.eslintrc.json

+    "react/react-in-jsx-scope": "off",
+    "react/jsx-uses-react": "off",


This was added to remove a warning from the nx merge airbnb rules. Basically it gives an error if you use jsx in a file without importing React at the top of each file 🤮

Okay I actually removed this since I rebased on Josh's PR which fixed these eslint rules!

marcellmueller · 2024-05-03T23:24:11Z

bciers/apps/registration/app/components/form/widgets/readOnly/ReadOnlyComboBoxWidget.tsx

@@ -10,6 +10,7 @@ const ReadOnlyComboBoxWidget: React.FC<WidgetProps> = ({
  ];
  const selectedValue =
    fieldSchema &&
+    value &&


Found this bug while testing the empty state of the read only widget. Without this added it would show a selected value even if there was no value 😱

😱 nice find!

bciers/apps/registration/tests/components/form/widgets/MultiSelectWidget.test.tsx

marcellmueller · 2024-05-06T15:43:19Z

bciers/package.json

+    "reg:coverage": "nx run registration:test --coverage",
+    "reg:test": "nx run registration:test"


hot reloading is currently not working. To enable it I believe we will have to update nx, so I decided to make it's own issue:

#1610

marcellmueller · 2024-05-06T16:26:23Z

bciers/apps/registration/tests/components/form/widgets/URLWidget.test.tsx

+    expect(screen.queryByText(urlErrorMessage)).toBeNull();
+  });
+
+  // Todo: Fix bug preventing required field validation


The fix I did ages ago for the URL widget empty string error prevents it from being set to a required field. There is more context in this bug ticket though it's something we should handle properly in the future.

#1612

marcellmueller · 2024-05-06T16:27:26Z

bciers/apps/registration/tests/components/form/widgets/FileWidget.test.tsx

+  },
+};
+
+describe("RJSF FileWidget", () => {


Feels like this could be more thoroughly tested. What are we missing here?

should allow user to select a file for upload, then remove it?

should display an error when trying to upload files greater than ? (iirc, when Brianna implemented file upload, she added a file size limit)

(if we have restrictions on file type in place?) should display an error message when an "undesired" file (e.g., .exe) is uploaded

if we want to be really on the ball, should allow file types of .kml and .kmz to be uploaded

Okay I've done what I can here but some things to note as our FileWidget is super basic at the moment:

There is no feature to remove the file. I added a test to verify that we can replace the current file though.

The accept functionality for file types just blocks them from being selected in the document browser and we don't currently have any built in validation messages. I added a test that verifies that the input won't accept a non-pdf.

I added a test to confirm that the accept option would accept .kml/.kmz. I wasn't sure if you meant you actually wanted that enabled for the file upload field on the operations form. It would be easy to do but I only added a test.

Added a couple tests for the 20mb maximum, one to verify it won't accept that input and one that uses spyOn for the window alert and makes sure it gets called.

the test cases for FileWidget are very comprehensive now - great work @BCerki and @marcellmueller !

andrea-williams

Fantastic work Marcel!

andrea-williams · 2024-05-08T21:05:16Z

bciers/apps/registration/app/components/form/widgets/readOnly/ReadOnlyComboBoxWidget.tsx

@@ -10,6 +10,7 @@ const ReadOnlyComboBoxWidget: React.FC<WidgetProps> = ({
  ];
  const selectedValue =
    fieldSchema &&
+    value &&


😱 nice find!

andrea-williams · 2024-05-08T22:21:34Z

bciers/apps/registration/tests/components/form/widgets/ComboBox.test.tsx

+
+export const comboBoxFieldSchema = {
+  type: "object",
+  required: ["comboBoxTestField"],


likely just me being an idiot, but why is the value of required an array of strings? I would have expected a bool

Take a look at any of our actual form schemas. In JSONSchema format this is an array of all required fields in an object. If it is a nested object then it requires it's own array for those fields.

andrea-williams · 2024-05-08T22:28:18Z

bciers/apps/registration/tests/components/form/widgets/ComboBox.test.tsx

+    expect(comboBoxInput.value).toBe("Option 3");
+  });
+
+  it("should allow clearing a combo box value", async () => {


feels like we should have an additional test here to assert that multiple values can be selected simultaneously in the comboBox?

re-review: not seeing a test case for multiple simultaneous values...?

I don't think we can select more than one option in the comboBox. We can in the MultiSelectWidget

andrea-williams · 2024-05-08T22:30:49Z

bciers/apps/registration/tests/components/form/widgets/FileWidget.test.tsx

+  "data:application/pdf;name=dummy.pdf;base64,ZHVtbXk=";
+const mockFile = new File(["test"], "test.pdf", { type: "application/pdf" });
+
+vi.resetAllMocks();


nit: is this needed here when it's also called in beforeEach()?

andrea-williams · 2024-05-08T23:19:18Z

bciers/apps/registration/tests/components/form/widgets/FileWidget.test.tsx

+  },
+};
+
+describe("RJSF FileWidget", () => {


should allow user to select a file for upload, then remove it?

should display an error when trying to upload files greater than ? (iirc, when Brianna implemented file upload, she added a file size limit)

(if we have restrictions on file type in place?) should display an error message when an "undesired" file (e.g., .exe) is uploaded

if we want to be really on the ball, should allow file types of .kml and .kmz to be uploaded

bciers/apps/registration/tests/components/form/widgets/MultiSelectWidget.test.tsx

andrea-williams · 2024-05-09T00:07:55Z

bciers/apps/registration/tests/components/form/widgets/TextWidget.test.tsx

+    expect(input).toHaveValue("test");
+  });
+
+  it("should only allow entering numbers in a number field", async () => {


I think the other use case we might need in the future is decimal values (e.g., 23.5)
and in the case of reporting in particular, I'd expect there will be users typing in large values like "1,320,000" so maybe a test with commas in the input value?

Sounds good add a should not allow entering commas in a number field test.

andrea-williams · 2024-05-09T00:11:28Z

bciers/apps/registration/tests/components/form/widgets/URLWidget.test.tsx

+  it("should trigger validation error for invalid URL", async () => {
+    render(<FormBase schema={urlFieldSchema} uiSchema={urlFieldUiSchema} />);
+    const input = screen.getByLabelText(urlLabelRequired);
+    await userEvent.type(input, "example.com");


how hard/easy is it to change this behaviour? IMO, "example.com" should be considered a valid URL. As a naive user, I wouldn't expect to have to specify "https://www.example.com" - I don't consider that to be the norm anymore.
I would call "example" or ".com" invalid URLs.

{end rant}

Haha I pushed to create our own custom validation for this because I thought forcing users to add http:// or https:// before a url was silly. Though I got overridden pre-mvp. There is a ticket for it, it's out of scope for this PR.

BCerki

These tests are super comprehensive, great work! I've reviewed everything except the file widget, which I'm going to spend a little more time with today. Re my comments about adding test coverage--I've mentioned things I noticed aren't covered, but I'm not sure they do need to be covered (e.g. custom onBlur). Just wanted to points out gaps in case you hadn't already considered them.

bciers/apps/registration/tests/components/form/widgets/CheckboxWidget.test.tsx

BCerki · 2024-05-08T21:46:43Z

bciers/apps/registration/tests/components/form/widgets/CheckboxWidget.test.tsx

+    expect(checkbox).toBeChecked();
+  });
+
+  it("should allow unchecking the checkbox", async () => {


Missing tests for:

when field is required

A boolean field can't be required. It's either true or it's false.

I think if it's optional users can just not select anything and the form will still submit. I added a test for when it's required and I did get an error message

good call @BCerki - we will need the option to make checkboxes required when external users are accepting terms & conditions (or the equivalent for BCIERS - more likely to be something like "I confirm that the information I've submitted is accurate")

BCerki · 2024-05-08T21:49:11Z

bciers/apps/registration/tests/components/form/widgets/FileWidget.test.tsx

+  "data:application/pdf;name=dummy.pdf;base64,ZHVtbXk=";
+const mockFile = new File(["test"], "test.pdf", { type: "application/pdf" });
+
+vi.resetAllMocks();


I don't think this is needed here; it's in already in beforeEach

BCerki · 2024-05-08T22:04:05Z

bciers/apps/registration/tests/components/form/widgets/MultiSelectWidget.test.tsx

+    const selectedOption1 = screen.getByRole("button", { name: "Option 2" });
+    const selectedOption2 = screen.getByRole("button", { name: "Option 3" });


Suggest naming the variables the same number as the option to avoid confusion

BCerki · 2024-05-08T22:14:10Z

bciers/apps/registration/tests/components/form/widgets/MultiSelectWidget.test.tsx

+    const optionCancelIcon = screen.getByTestId("CloseIcon");
+
+    await userEvent.click(optionCancelIcon);


I would have expected this to fail because if there are two chips, then getByTestId would choke because there are two close icons. But it seems to have worked?

TestId is CancelIcon

bciers/apps/registration/tests/components/form/widgets/TextWidget.test.tsx

bciers/apps/registration/tests/components/form/widgets/URLWidget.test.tsx

BCerki · 2024-05-09T15:41:05Z

bciers/apps/registration/tests/components/form/widgets/readOnly/ReadOnlyComboBoxWidget.test.tsx

+} from "@/tests/components/form/widgets/ComboBox.test";
+
+describe("RJSF ReadOnlyComboBoxWidget", () => {
+  it("should render a combo box field", async () => {


This additionally checks that the form data comes through correctly, so suggest adding something about that to the test name

BCerki · 2024-05-09T15:41:22Z

.../apps/registration/tests/components/form/widgets/readOnly/ReadOnlyMultiSelectWidget.test.tsx

+} from "@/tests/components/form/widgets/MultiSelectWidget.test";
+
+describe("RJSF ReadOnlyMultiSelectWidget", () => {
+  it("should render a multi select field", async () => {


same, the formData part isn't captured in the test name

bciers/apps/registration/tests/components/form/widgets/PostalCodeWidget.test.tsx

BCerki

Anything marked with the rocket ship emoji I've implemented

BCerki · 2024-05-09T16:27:06Z

bciers/apps/registration/tests/components/form/widgets/FileWidget.test.tsx

+
+const fileFieldLabel = "FileWidget test field";
+const fileLabelRequired = `${fileFieldLabel}*`;
+export const dummyDataUri =


Can we please use something other than "dummy"? "Mock," "sample," whatever

bciers/apps/registration/tests/components/form/widgets/FileWidget.test.tsx

BCerki · 2024-05-09T22:30:39Z

bciers/apps/registration/tests/components/form/widgets/FileWidget.test.tsx

+    const input = screen.getByLabelText(fileLabelRequired);
+    await userEvent.upload(input, mockFile);
+
+    actionHandler.mockResolvedValueOnce({});


Why do we need to mock the actionHandler here? If I remember right, the file doesn't get uploaded until the user clicks submit, so I wouldn't expect the actionHandler to fire at this point

BCerki · 2024-05-09T22:46:21Z

bciers/apps/registration/tests/components/form/widgets/FileWidget.test.tsx

+      screen.queryByRole("button", { name: "Upload attachment" }),
+    ).not.toBeInTheDocument();
+  });
+});


Possible tests to add coverage:

something for this: We need to store the value in state to prevent loosing the value when user switches between tabs

file input is correctly hidden

required

replacing an uploaded file when there was one in the formData (I already see a test for replacing one before it got submitted)

it looks sort of like the file widget is set up to upload more than one file (processFiles function) but we only allow uploading one

I didn't find a way to test tab-switching-- Document.visibilityState was the closest and that's not really it. Did the rest

BCerki · 2024-05-13T20:40:01Z

bciers/apps/registration/tests/components/form/widgets/CheckboxWidget.test.tsx

+    expect(checkbox).toBeChecked();
+  });
+
+  it("should allow unchecking the checkbox", async () => {


I think if it's optional users can just not select anything and the form will still submit. I added a test for when it's required and I did get an error message

BCerki · 2024-05-13T21:54:10Z

bciers/apps/registration/tests/components/form/widgets/MultiSelectWidget.test.tsx

+    const optionCancelIcon = screen.getByTestId("CloseIcon");
+
+    await userEvent.click(optionCancelIcon);


TestId is CancelIcon

BCerki · 2024-05-13T22:33:18Z

bciers/apps/registration/tests/components/form/widgets/PhoneWidget.test.tsx

+      "12345678",
+    );
+  });
+});


The error for this case is about formatting, which isn't accurate, so created this ticket: https://github.com/orgs/bcgov/projects/122/views/2?filterQuery=&pane=issue&itemId=62787538

BCerki · 2024-05-13T22:38:15Z

bciers/apps/registration/tests/components/form/widgets/RadioWidget.test.tsx

+    expect(optionYes).not.toBeChecked();
+    expect(optionNo).toBeChecked();
+    expect(optionMaybe).not.toBeChecked();
+  });


Did the required errors part, not sure if the custom functions need tests; they look like they're coming from node_modules

bciers/playwright-report/trace/sw.bundle.js

BCerki · 2024-05-14T16:27:12Z

bciers/apps/registration/tests/helpers/form.ts

+  expect(inputBorderElement).toHaveStyle(errorStyle);
+};
+
+export const checkNoValidationErrorIsTriggered = async () => {


Sometimes this causes test errors, so not using it everywhere yet

andrea-williams

my brain is kaput - will continue reviewing this tomorrow

andrea-williams · 2024-05-16T00:13:12Z

bciers/apps/registration/tests/components/form/widgets/CheckboxWidget.test.tsx

+    expect(checkbox).toBeChecked();
+  });
+
+  it("should allow unchecking the checkbox", async () => {


good call @BCerki - we will need the option to make checkboxes required when external users are accepting terms & conditions (or the equivalent for BCIERS - more likely to be something like "I confirm that the information I've submitted is accurate")

andrea-williams · 2024-05-16T00:18:53Z

bciers/apps/registration/tests/components/form/widgets/ComboBox.test.tsx

+    expect(comboBoxInput.value).toBe("Option 3");
+  });
+
+  it("should allow clearing a combo box value", async () => {


re-review: not seeing a test case for multiple simultaneous values...?

andrea-williams · 2024-05-16T22:56:34Z

bciers/apps/registration/tests/components/form/widgets/EmailWidget.test.tsx

+      <FormBase schema={emailFieldSchema} uiSchema={emailFieldUiSchema} />,
+    );
+    const input = screen.getByLabelText(emailLabelRequired);
+    await userEvent.type(input, "test.email@test.com");


This input doesn't match the test description of a "subdomain" - I propose changing the input to test@email.test.com

andrea-williams · 2024-05-16T23:01:03Z

bciers/apps/registration/tests/components/form/widgets/FileWidget.test.tsx

+  },
+};
+
+describe("RJSF FileWidget", () => {


the test cases for FileWidget are very comprehensive now - great work @BCerki and @marcellmueller !

andrea-williams · 2024-05-16T23:03:55Z

bciers/apps/registration/tests/components/form/widgets/PostalCodeWidget.test.tsx

+
+    await userEvent.click(submitButton);
+
+    expect(screen.getByText("Format should be A1A 1A1")).toBeVisible();


Horrible thought: if we're accepting American phone numbers, should we also be accepting American zip codes...? (This thought makes me sad.)

😱 I'll ask the designers

andrea-williams · 2024-05-16T23:10:27Z

bciers/apps/registration/tests/components/form/widgets/MultiSelectWidget.test.tsx

+      screen.getByPlaceholderText("Select regulated products..."),
+    ).toBeVisible();
+  });
+  it("should use the enum values as the names if no enumNames are provided, use the enum values as the names", async () => {


this test name seems a little repetitive, a little bit

good point point

andrea-williams

Not able to run the tests locally because something wacky is going on with my local git. (Grrrr).

But code LGTM and it's the end of day before a long weekend so I'm feeling especially reckless.

BCerki · 2024-05-17T15:45:26Z

Not able to run the tests locally because something wacky is going on with my local git. (Grrrr).

But code LGTM and it's the end of day before a long weekend so I'm feeling especially reckless.

I too like to live dangerously

marcellmueller commented May 3, 2024

View reviewed changes

bciers/apps/registration/tests/components/form/widgets/MultiSelectWidget.test.tsx Show resolved Hide resolved

marcellmueller commented May 6, 2024

View reviewed changes

marcellmueller marked this pull request as ready for review May 6, 2024 16:14

marcellmueller commented May 6, 2024

View reviewed changes

andrea-williams reviewed May 9, 2024

View reviewed changes

marcellmueller force-pushed the 1222-vitest-widgets branch from 809b074 to 0d6d99c Compare May 9, 2024 15:14

BCerki reviewed May 9, 2024

View reviewed changes

BCerki force-pushed the 1222-vitest-widgets branch from 48d615c to a0e9971 Compare May 13, 2024 22:48

BCerki reviewed May 13, 2024

View reviewed changes

github-advanced-security bot found potential problems May 13, 2024

View reviewed changes

bciers/playwright-report/trace/sw.bundle.js Fixed Show fixed Hide fixed

BCerki force-pushed the 1222-vitest-widgets branch from fc0c845 to b08b4f6 Compare May 14, 2024 16:24

BCerki reviewed May 14, 2024

View reviewed changes

BCerki force-pushed the 1222-vitest-widgets branch 3 times, most recently from 4a3a31c to 666a911 Compare May 15, 2024 16:59

andrea-williams reviewed May 16, 2024

View reviewed changes

BCerki force-pushed the 1222-vitest-widgets branch from 666a911 to f75bbd4 Compare May 16, 2024 15:56

andrea-williams reviewed May 16, 2024

View reviewed changes

marcellmueller added 9 commits May 16, 2024 16:28

test: move widget pr from nx rebase

c9a297e

chore: add fixes from widget testing pr

8378226

chore: fix file widget test

6666be1

chore: add vitest coverage

d30de28

chore: reduce duplication for sonarcloud

f206f01

chore: add comment to url widget test

f41641f

test: add number widget comma test

7e87a66

test: add select multiple combo box values test

8408dcb

chore: add extra file widget tests

9ae8d57

marcellmueller and others added 5 commits May 16, 2024 16:28

chore: remove eslint disable react in scope

342499b

chore: fix unhandled error in OperatorSearchWidget

41d4524

test: add more coverage

9c7364b

chore: cleanup

996e9c8

chore: apply suggestions from code review

7916473

BCerki force-pushed the 1222-vitest-widgets branch from f75bbd4 to 7916473 Compare May 16, 2024 23:28

andrea-williams approved these changes May 17, 2024

View reviewed changes

BCerki merged commit f0f5345 into develop May 17, 2024
23 of 24 checks passed

BCerki deleted the 1222-vitest-widgets branch May 17, 2024 15:45

		"react/react-in-jsx-scope": "off",
		"react/jsx-uses-react": "off",

		"reg:coverage": "nx run registration:test --coverage",
		"reg:test": "nx run registration:test"

		const selectedOption1 = screen.getByRole("button", { name: "Option 2" });
		const selectedOption2 = screen.getByRole("button", { name: "Option 3" });

		const optionCancelIcon = screen.getByTestId("CloseIcon");

		await userEvent.click(optionCancelIcon);


		await userEvent.click(submitButton);

		expect(screen.getByText("Format should be A1A 1A1")).toBeVisible();

1222 vitest widgets #1605

1222 vitest widgets #1605

Conversation

marcellmueller commented May 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcellmueller May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrea-williams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BCerki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BCerki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BCerki May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BCerki May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrea-williams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrea-williams left a comment

Choose a reason for hiding this comment

BCerki commented May 17, 2024

marcellmueller commented May 3, 2024 •

edited

Loading

marcellmueller May 9, 2024 •

edited

Loading

BCerki May 14, 2024 •

edited

Loading

BCerki May 13, 2024 •

edited

Loading