Mocking Callbacks And Components: Redundant button presence check and unnecessary async keyword in CustomButton tests

[dekr1sh]

Checks

• [X] This is not a duplicate of an existing issue (please have a look through our open issues list to make sure)
• [X] I have thoroughly read and understand The Odin Project Contributing Guide
• [X] Would you like to work on this issue?

Describe your suggestion

Issue:

1. Redundant button presence check:

   • The test for rendering the button with the text "Click me" uses screen.getByRole("button", { name: "Click me" }) to find the button. Since getByRole will throw an error if the button is not found, the additional expect(button).toBeInTheDocument() is redundant.

2. Unnecessary async keyword:

   • In the test that verifies the onClick function is not called when the button isn't clicked, the async keyword is used. However, there are no asynchronous operations (like await or asynchronous events) in this test, making the async keyword unnecessary.

Proposed Solution:

1. Remove redundant button presence check:

   Before:

   const button = screen.getByRole("button", { name: "Click me" });
   expect(button).toBeInTheDocument();
   

   After:

   const button = screen.getByRole("button", { name: "Click me" });
   expect(button).toHaveTextContent("Click me");
   

2. Remove the async keyword where not needed:

   Before:

   it("should not call the onClick function when it isn't clicked", async () => {
    const onClick = vi.fn();
    render(<CustomButton onClick={onClick} />);
   
    expect(onClick).not.toHaveBeenCalled();
   });
   

   After:

   it("should not call the onClick function when it isn't clicked", () => {
    const onClick = vi.fn();
    render(<CustomButton onClick={onClick} />);
   
    expect(onClick).not.toHaveBeenCalled();
   });
   

Path

Node / JS

Lesson Url

https://www.theodinproject.com/lessons/node-path-react-new-mocking-callbacks-and-components

(Optional) Discord Name

de_kr1sh

(Optional) Additional Comments

No response

[JoshDevHub]

Thank you for making this issue @dekr1sh -- and thank you for writing it in such a detailed way!

I'll ping our @TheOdinProject/javascript team to get their opinions on your proposed changes.

[01zulfi]

1. Let's replace getByRole with queryByRole and keep the expect statement
2. Agreed

[mao-sz]

@01zulfi @dekr1sh The async thing is fine to fix, but the getByRole thing should be left as is,

query* is only for negative assertions to prevent an error being thrown. Positive query* queries that fail will only return null, and can only be tested against that, so the test output is minimal and unhelpful. Positive get* queries call screen.debug() when they fail and throw, which is significantly more helpful.

get* queries still ought to have assertions if asserting something. While redundant when the code is actually run, explicit assertions are definitely not redundant to developers. Especially when get* queries don't always come with assertions (e.g. grabbing an element to then interact with it). i.e. get* queries should ideally just be used as queries and not only sometimes double as assertions. Assertions should still be explicit.

[mao-sz]

Reopening for assignment. Please do not change any of the actual queries (see above comment).

Acceptance criteria

• [ ] Remove the unnecessary async keyword from the last test callback in the code block.

Comment below if you'd like to be assigned to work on this issue. Please do not open a PR unless you've been assigned by a maintainer.

[fadotti]

May I be assigned to this issue?

[mao-sz]

Go for it