Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renu - Workshop Feedback #8

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Renu - Workshop Feedback #8

wants to merge 2 commits into from

Conversation

BenSheridanEdwards
Copy link
Collaborator

No description provided.

Copy link
Collaborator Author

@BenSheridanEdwards BenSheridanEdwards left a comment

Choose a reason for hiding this comment

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

A real mixed bag here Renu. You're nailing our architecture and conventions but you really let yourself down because you rushed without checking your work. It's your biggest problem as a developer and it showcased itself again here.

  • Lack of understanding of the asynchronous aspect of testing.
  • You made careless errors in your test names and your last <Form /> test.
  • Numerous tests are failing, worse of all it shows you didn't run your tests before submitting.

Run your tests, look at my feedback, feel how you let yourself down here, and finally absorb the lesson that rushing will only lead to mistakes and low-quality output.

I know you can be a great developer, but right now you need to let the pain of this be the wake-up call, to finally commit yourself to slowing down and becoming a professional.

Architecture: 10/10
Followed conventions: 9/10
Clean Implementation: 8/10
Percentage of passing tests: 66%

Tests: 8 failed, 16 passed, 24 total

import { Accordion, AccordionProps } from "./Accordion";

const renderComponent = (additionalProps?: Partial<AccordionProps>) =>{
const defaultProps = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm surprised you aren't getting a type error when you try and pass this object into the Accordion component.

But I just checked and nothing comes up. I thought you had to do:

const defaultProps: AccordionProps =

it("renders the children correctly", async () => {
renderComponent();
userEvent.click(screen.getByRole("button", {name: "My title"}))
expect(await screen.findByRole("heading", {name: "Accordion content"})).toBeInTheDocument();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This works, but I would have personally gone findByText on this one. Usually, findByRole is the better option but you're more concerned the text/content is right, not the element itself.

Still, keep it as is. It's good work.

it("does not render the children unless the button in clicked", async () => {
renderComponent();
expect(screen.queryByRole("heading", {name: "Accordion content"})).not.toBeInTheDocument();
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great test file, the only test case I would add would be to test the content is hidden after a second click.


it("selects the first option in the list by default", () => {
renderComponent();
expect(screen.getByRole("menu")).toHaveValue("Option 1");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test is failing with:

TestingLibraryElementError: Unable to find an accessible element with the role "menu"

Don't you want combobox?

  it("selects the first option by default", () => {
    renderComponent();
    expect(screen.getByRole("combobox")).toHaveValue("Option 1");
  });

This passes


it("selects the options correctly", () => {
renderComponent();
userEvent.click(screen.getByRole("menuitem", { name: "Option 2" }));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same issue as above.

Since this is a native select, you needed to use userEvent.selectOptions to change the dropdown.

  it("allows a user to select an option", async () => {
    renderComponent();

    expect(screen.getByRole("combobox")).toHaveValue("Option 1");

    await userEvent.selectOptions(screen.getByRole("combobox"), "Option 2");

    expect(screen.getByRole("combobox")).toHaveValue("Option 2");
  });

userEvent.type(screen.getByRole("textbox", {name: "Name"}), "Renu")
userEvent.type(screen.getByRole("textbox", {name: "Name"}), "[email protected]")
userEvent.click(screen.getByRole("button", {name: "Submit"}));
expect(handleFormSubmitMock).toHaveBeenCalledWith({name: "[email protected]"})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test was a real shame, as it was clearly rushed. Really look at your test Renu. Take the time to read and absorb the consequences of going too fast. Read every line before looking below.

When you rush, you make mistakes. In this test, you made a number of mistakes.

Compare your test to mine below

  it("calls the onSubmit function with the correct data when the form is filled in and the submit button is clicked", async () => {
    renderComponent();
    await userEvent.type(screen.getByRole("textbox", { name: "Name" }), "Renu");
    await userEvent.type(
      screen.getByRole("textbox", { name: "Email" }),
      "[email protected]",
    );
    await userEvent.click(screen.getByRole("button", { name: "Submit" }));
    expect(handleFormSubmitMock).toHaveBeenCalledWith({
      name: "Renu",
      email: "[email protected]",
    });
  });

expect(handleCloseMock).toHaveBeenCalled()
});

it("renders the cancel button correctly", () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're not testing it renders, you're testing the onClose is called when clicked.

expect(handleCloseMock).toHaveBeenCalled()
});

it("renders the confirm button correctly", () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Again you're testing the handleConfirm is being called.

renderComponent({showActions: true});
userEvent.click(screen.getByRole("button", {name: "Confirm"}));
expect(handleConfirmMock).toHaveBeenCalled()
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These last three tests are all failing without async await

userEvent.type(await screen.findByRole("textbox", {name: "Name"}), "Renu")
userEvent.type(await screen.findByRole("textbox", {name: "Email"}), "[email protected]")
userEvent.click(screen.getByRole("button", {name: "Submit"}));
expect(await screen.findByText("Hello Renu, your email [email protected] has been submitted!"))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test is failing as you're not waiting for the userEvent.type to input your text. You're also using findBy when you should be using getBy.

When you aren't waiting for data to come into your component for it to render, it will always be available, it's only when you are waiting for something async to happen so the content changes that you need findBy

  it("displays the toast message when the form is submitted", async () => {
    renderComponent();
    await userEvent.click(screen.getByRole("button", { name: "Open Form" }));

    await userEvent.type(screen.getByRole("textbox", { name: "Name" }), "Renu");
    await userEvent.type(
      screen.getByRole("textbox", { name: "Email" }),
      "[email protected]",
    );

    await userEvent.click(screen.getByRole("button", { name: "Submit" }));

    expect(
      await screen.findByText(
        "Hello Renu, your email [email protected] has been submitted!",
      ),
    );
  });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants