Skip to content

Conversation

Shelob9
Copy link
Contributor

@Shelob9 Shelob9 commented Jun 5, 2019

This was something I needed to do today, and it wasn't documented, so I wanted to share.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Nobody merge this yet. I have thoughts and feedback that I'll give when I'm back at my keyboard.

@kentcdodds
Copy link
Member

Thanks for this. I think it's an important thing to include in the docs. However I want to change a few things. Here's how I would write it:

// button.js
const Button = ({onClick, children}) => <button onClick={onClick}>{children}</button>

// __tests__/button.js
test('calls onClick prop when clicked', () => {
  const handleClick = jest.fn()
  const {getByText} = render(<Button onClick={handleClick}>Click Me</Button>)
  fireEvent.click(getByText(/click me/i))
  expect(handleClick).toHaveBeenCalledTimes(1)
})

I'll add comments to the lines in your original sample to explain why I made the changes that I did.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Here are my notes. What do you think?


```jsx
const Button = ({onClick,children}) => <button onClick={onClick}>{children}</button>
describe('Button component', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to avoid nesting. We already know that this is about the button component because we're in the file that's testing the button component.

```jsx
const Button = ({onClick,children}) => <button onClick={onClick}>{children}</button>
describe('Button component', () => {
let onClick;
Copy link
Member

Choose a reason for hiding this comment

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

Sharing this variable through a bunch of tests makes it harder to maintain long-term. This isn't quite the same thing, but it's similar to what I talk about in Test Isolation with React.

beforeEach(() => {
onClick = jest.fn();
});
afterEach(cleanup);
Copy link
Member

Choose a reason for hiding this comment

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

We can just assume that people have the cleanup stuff working in their setup.

it('Fires on click event', () => {
const { container } = render(
<Button
label={'With Callback'}
Copy link
Member

Choose a reason for hiding this comment

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

The Button component does nothing with the label prop so I removed that.

Trigger Alert
</Button>
);
const input = container.querySelector('button');
Copy link
Member

@kentcdodds kentcdodds Jun 5, 2019

Choose a reason for hiding this comment

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

We're not looking for an input, and we should avoid using and suggesting querySelector and instead go with a more semantic selector like getByText.

);
const input = container.querySelector('button');
fireEvent.click(input);
expect(onClick.mock.calls[0].length).toBe(1);
Copy link
Member

Choose a reason for hiding this comment

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

There's a built-in assertion for this, so I switched to that one.

Copy link
Contributor Author

@Shelob9 Shelob9 left a comment

Choose a reason for hiding this comment

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

This is great. Makes more sense this way.

@Shelob9
Copy link
Contributor Author

Shelob9 commented Jun 6, 2019

I think that the docs could use a similar example for onChange. Should this be a new PR?

These are the examples abstracted out from what I'm actually working on.

An input component with some logic inside of it:

// input.js
const Input = ({ onChange, value }) => {
  const changeHandler = event => onChange(event.target.value);
  return (
    <input data-testid="input-test" onChange={changeHandler} value={value} />
  );
};

Tests that change handler is called and that is called with the right value:

// __tests__/input.js
describe("text input", () => {
  afterEach(cleanup);
  test("calls onChange prop when changed", () => {
    const onChange = jest.fn();
    const { getByTestId } = render(<Input onChange={onChange} value={""} />);
    fireEvent.change(getByTestId("input-test"), {
      target: { value: "New Value" }
    });
    expect(onChange).toHaveBeenCalledTimes(1);
  });

  test("When input is changed, the new value, not the event is returned by", () => {
    const onChange = jest.fn();
    const { getByTestId } = render(<Input onChange={onChange} value={""} />);
    fireEvent.change(getByTestId("input-test"), {
      target: { value: "New Value" }
    });
    expect(onChange).toHaveBeenCalledWith("New Value");
  });
});

#138 (comment)

These are nested with cleanup(). I got tripped up very badly when moving tests from Enzyme to react-testing-library, beacuse I totally missed why it was important in groups of tests.

@kentcdodds
Copy link
Member

I think that's a great idea. Let's do a different PR for the onChange. One thing I'll mention is we really want to avoid using TestId queries in any of the examples if we can help it! thanks!

@Shelob9
Copy link
Contributor Author

Shelob9 commented Jun 8, 2019

Sounds great. I'll copy that into a real PR, and update it to query by label or text once this is merged.

@afontcu
Copy link
Member

afontcu commented Jul 27, 2019

Hi @Shelob9! Any chance you could work on Kent's feedback and update the PR? :)

@Shelob9
Copy link
Contributor Author

Shelob9 commented Jul 27, 2019

@afontcu Thanks for the nudge, this got lost in all things. I'm updating now.

@afontcu
Copy link
Member

afontcu commented Aug 1, 2019

Now that I look at the issue with fresh eyes, I'm wondering if we should use a React-based example to illustrate an example in DOM Testing Library docs?

We could either (1) move the example to React docs, and add demos for other frameworks, or (2) use plain DOM example with document.createElement and so on.

Thoughts? 🙌

@kentcdodds
Copy link
Member

we should use a React-based example to illustrate an example in DOM Testing Library docs?

Yeah. I don't think we should use the react stuff with DOM Testing Library. Let's make it as vanilla as possible.

move the example to React docs, and add demos for other frameworks, or (2) use plain DOM example with document.createElement and so on.

I like both ideas. We should have demos/examples for all frameworks, including vanilla (document.createElement) stuff for DOM Testing Library.

@alexkrolick
Copy link
Collaborator

If someone wants to add a generic version, add a codetab for it. I wrapped the existing example in a React tab.

Screen Shot 2020-10-01 at 12 13 20 PM

@alexkrolick alexkrolick merged commit 14f5755 into testing-library:master Oct 1, 2020
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.

4 participants