Skip to content

Tests don't fail gracefully when using wait #78

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

Closed
RyanAtViceSoftware opened this issue May 7, 2018 · 5 comments · Fixed by #79
Closed

Tests don't fail gracefully when using wait #78

RyanAtViceSoftware opened this issue May 7, 2018 · 5 comments · Fixed by #79

Comments

@RyanAtViceSoftware
Copy link
Contributor

  • react-testing-library version:
    "react-testing-library": "^2.5.1",
  • react version:
    "react": "^16.3.0",
  • node version:
    v8.9.4
  • npm (or yarn) version:
    5.6.0

Relevant code or config:

// Test
import sinon from "sinon"
import { Simulate, wait } from "react-testing-library"
import { client } from "../../../_old/graphql/apolloClient"
import { mountApp } from "../../../test/testBehaviors"

jest.mock("../../../_old/graphql/apolloClient")

afterEach(() => {
  client.resetStore()
})

describe("Given we load the App", () => {
  describe("When data is returned and there is no user ", () => {
    describe("And we enter a valid username and password and click submit", () => {
      it("Then we call the login mutation with correct arguments and are redirected to the search page", async () => {
        const userResolverStub = sinon.stub()

        userResolverStub.onFirstCall().returns(null)

        userResolverStub.onSecondCall().returns({
          id: "fakeId",
          email: "fakeEmail",
          role: "fakeRole",
          firstName: "fakeFirstName",
          lastName: "fakeLastName",
          algoliaSearchApiKey: "fakeAlgoliaSearchApiKey"
        })

        const loginMuationMock = sinon.expectation.create("loginMutationMock")

        loginMuationMock
          .withArgs(sinon.match.any, {
            input: { email: "[email protected]", password: "fakePassword" }
          })
          .once()
          .returns({
            user: {
              id: "fakeId",
              email: "fakeEmail",
              role: "fakeRole",
              firstName: "fakeFirstName",
              lastName: "fakeLastName",
              algoliaSearchApiKey: "fakeAlgoliaSearchApiKey"
            },
            company: {
              hasPurchasing: true
            }
          })

        const mocks = {
          User: userResolverStub,
          Mutation: () => ({
            // This needed to match the name of the mutation in MutationType.js
            login: loginMuationMock
          })
        }

        const { getByTestId } = mountApp({ mocks })

        await wait(() => {
          const emailTextBox = getByTestId("emailTextBox")

          emailTextBox.value = "[email protected]"

          Simulate.change(emailTextBox)
        })

        await wait(() => {
          const passwordTextBox = getByTestId("passwordTextBox")

          passwordTextBox.value = "fakePassword"

          Simulate.change(passwordTextBox)
        })

        await wait(() => {
          const loginForm = getByTestId("loginButton")

          Simulate.submit(loginForm)
        })

        await wait(() => {
          loginMuationMock.verify()
          expect(getByTestId("search-page")).toBeTruthy()
        })
      })
    })
  })
})

// Component
import _ from "lodash"
import React, { Component } from "react"
import { Link, withRouter } from "react-router-dom"
import styled from "styled-components"
import { withLogin } from "../graphql/mutations/loginMutation"
import { ERRORS } from "../../../../errors"
import { Routes } from "../../../../Routes/index"
import Alert from "../../../../sharedComponents/Alert/index"
import BackgroundButton, {
  Colors
} from "../../../../sharedComponents/forms/BackgroundButton"
import Input from "../../../../sharedComponents/forms/Input"
import GuestContainer from "../../../../sharedContainers/GuestContainer/index"

const LoginStyled = styled(GuestContainer)`
  .password-container {
    position: relative;

    .forgot-password-link {
      position: absolute;
      right: 0;
      top: 33px;
      font-size: 10px;
      font-weight: 100;
      text-transform: uppercase;
    }
  }
`
class Login extends Component {
  state = {
    email: "",
    password: ""
  }

  handleChange = (name, value) => this.setState({ [name]: value })

  login = () => {
    const { email, password } = this.state
    this.props
      .mutate({
        variables: { 
          input: { 
            email, 
            password: password + 'changed'  // <==== trying to break the test
          }} 
      })
      .then(() =>
        this.props.history.push(
          _.get(this.props, "location.state.from", Routes.ROOT)
        )
      )
      .catch(({ graphQLErrors }) => {
        switch (graphQLErrors[0].message) {
          case ERRORS.WrongEmailOrPassword:
            Alert.error("Email or password is incorrect")
            break
          default:
            break
        }
      })
  }

  handleSubmit = e => {
    e.preventDefault()
    this.login()
  }

  render() {
    const { email, password } = this.state
    return (
      <LoginStyled header="Welcome Back" subHeader="Your kitchen awaits">
        <form data-testid="login-form" onSubmit={this.handleSubmit}>
          <Input
            data-testid="emailTextBox"
            value={email}
            label="Email"
            onChange={e => this.handleChange("email", e.target.value)}
            autoFocus
          />

          <div className="password-container">
            <Input
              data-testid="passwordTextBox"
              value={password}
              type="password"
              label="Password"
              onChange={e => this.handleChange("password", e.target.value)}
            />

            <Link
              className="forgot-password-link"
              to={`${Routes.RESET}?email=${email}`}
            >
              Forgot password?
            </Link>
          </div>

          <BackgroundButton
            data-testid="loginButton"
            noHorizontalPadding
            color={Colors.PAPAYA}
            type="submit"
            disabled={!(email && password)}
          >
            Login
          </BackgroundButton>
        </form>
      </LoginStyled>
    )
  }
}

export default withLogin(withRouter(Login))

What you did:

I got the above test working and then I wanted to verify that it would break so I changed the password code like this password: password + 'changed' so that the test would fail and I could make sure the error was good and all that.

What happened:

The test failed eventually but it took a really long time and then didn't provided useful error feedback. We were instead getting a timeout error.

Reproduction:

I think this reproduces the issue. This seems to run for a really long time and never provide useful error feedback.

https://codesandbox.io/s/rjozql4jnm

Problem description:

We need the tests to fail fast and report back a useful error.

Suggested solution:

wait should run several times quickly and if it's not able to get a clean run with no errors then it should Promise.reject(error) with the error was thrown by the callback function.

@kentcdodds
Copy link
Member

Hi @RyanAtViceSoftware!

A few things:

  1. That codesandbox had a syntax error
  2. After fixing the syntax error, the tests took a long time to run, but they did pass
  3. I'm not sure why you're using wait so much, you shouldn't have to. Also, I would limit your wait callbacks to only what you're waiting for, not manipulating the DOM or firing events. This could lead to unexpected side-effects (maybe what you're experiencing

So where you have:

await wait(() => {
  const passwordTextBox = getByTestId("passwordTextBox")

  passwordTextBox.value = "fakePassword"

  Simulate.change(passwordTextBox)
})

You should probably do:

const passwordTextBox = await waitForElement(() => getByTestId("passwordTextBox"))
passwordTextBox.value = "fakePassword"

Simulate.change(passwordTextBox)

But what'd be even better is to not use wait or waitForElement if it's not necessary. From the look of things, those fields should all be available synchronously, so I don't think you need to use await at all.

@RyanAtViceSoftware
Copy link
Contributor Author

Hi Kent, thanks for the quick detailed reply.

Sorry for the sloppiness in the code sandbox link. The browser had locked up and I think it didn't save the last version. This is what I was trying to post.

https://codesandbox.io/s/rjozql4jnm

Anyway I was able to get the test to work by simplifying the wait calls and wanted to share the working code for future generations.

import sinon from "sinon"
import { Simulate, wait } from "react-testing-library"
import { client } from "../../../_old/graphql/apolloClient"
import { mountApp } from "../../../test/testBehaviors"

jest.mock("../../../_old/graphql/apolloClient")

afterEach(() => {
  client.resetStore()
})

describe("Given we load the App", () => {
  describe("When data is returned and there is no user ", () => {
    describe("And we enter a valid username and password and click submit", () => {
      it("Then we call the login mutation with correct arguments and are redirected to the search page", async () => {
        const userResolverStub = sinon.stub()

        userResolverStub.onFirstCall().returns(null)

        userResolverStub.onSecondCall().returns({
          id: "fakeId",
          email: "fakeEmail",
          role: "fakeRole",
          firstName: "fakeFirstName",
          lastName: "fakeLastName",
          algoliaSearchApiKey: "fakeAlgoliaSearchApiKey"
        })

        const loginMuationMock = sinon.expectation.create("loginMutationMock")

        loginMuationMock
          .withArgs(sinon.match.any, {
            input: { email: "[email protected]", password: "fakePassword" }
          })
          .once()
          .returns({
            user: {
              id: "fakeId",
              email: "fakeEmail",
              role: "fakeRole",
              firstName: "fakeFirstName",
              lastName: "fakeLastName",
              algoliaSearchApiKey: "fakeAlgoliaSearchApiKey"
            },
            company: {
              hasPurchasing: true
            }
          })

        const mocks = {
          User: userResolverStub,
          Mutation: () => ({
            // This needed to match the name of the mutation in MutationType.js
            login: loginMuationMock
          })
        }

        const { getByTestId } = mountApp({ mocks })

        await wait(() => {
          const emailTextBox = getByTestId("emailTextBox")

          emailTextBox.value = "[email protected]"

          Simulate.change(emailTextBox)

          const passwordTextBox = getByTestId("passwordTextBox")

          passwordTextBox.value = "fakePassword"

          Simulate.change(passwordTextBox)
        })

        const loginForm = getByTestId("loginButton")

        Simulate.submit(loginForm)

        await wait(() => {
          loginMuationMock.verify()
          expect(getByTestId("search-page")).toBeTruthy()
        })
      })
    })
  })
})

In case your interested in my confusion
I was getting the pattern wrong because I didn't understand the correct way to create .then() code blocks when using async \ await style promises.

@kentcdodds
Copy link
Member

I'm glad you got it working. But I still very strongly recommend against doing too much work in your wait callbacks. That code will run over and over and over again until it doesn't throw an error, so you could be simulating multiple change events on your input elements. It also will force you to wait for longer if there really is a problem. wait should be used sparingly and with care, otherwise you'll find yourself in a world of trouble.

RyanAtViceSoftware added a commit to RyanAtViceSoftware/react-testing-library that referenced this issue May 7, 2018
RyanAtViceSoftware added a commit to RyanAtViceSoftware/react-testing-library that referenced this issue May 7, 2018
@RyanAtViceSoftware
Copy link
Contributor Author

@kentcdodds I got my test simplified and I've shared that below for others who might be curious. I also created a documentation pull request that updates wait in the readme.md to add your recommended best practice. Thanks!

PR: #79

import sinon from "sinon"
import { Simulate, wait } from "react-testing-library"
import { client } from "../../../_old/graphql/apolloClient"
import { mountApp } from "../../../test/testBehaviors"

jest.mock("../../../_old/graphql/apolloClient")

afterEach(() => {
  client.resetStore()
})

describe("Given we load the App", () => {
  describe("When data is returned and there is no user ", () => {
    describe("And we enter a valid username and password and click submit", () => {
      it("Then we call the login mutation with correct arguments and are redirected to the search page", async () => {
        const userResolverStub = sinon.stub()

        userResolverStub.onFirstCall().returns(null)

        userResolverStub.onSecondCall().returns({
          id: "fakeId",
          email: "fakeEmail",
          role: "fakeRole",
          firstName: "fakeFirstName",
          lastName: "fakeLastName",
          algoliaSearchApiKey: "fakeAlgoliaSearchApiKey"
        })

        const loginMuationMock = sinon.expectation.create("loginMutationMock")

        loginMuationMock
          .withArgs(sinon.match.any, {
            input: { email: "[email protected]", password: "fakePassword" }
          })
          .once()
          .returns({
            user: {
              id: "fakeId",
              email: "fakeEmail",
              role: "fakeRole",
              firstName: "fakeFirstName",
              lastName: "fakeLastName",
              algoliaSearchApiKey: "fakeAlgoliaSearchApiKey"
            },
            company: {
              hasPurchasing: true
            }
          })

        const mocks = {
          User: userResolverStub,
          Mutation: () => ({
            // This needed to match the name of the mutation in MutationType.js
            login: loginMuationMock
          })
        }

        const { getByTestId } = mountApp({ mocks })

        let emailTextBox

        // Wait for tick after userResolverStub's first call
        await wait(() => {
          emailTextBox = getByTestId("emailTextBox")
        })

        emailTextBox.value = "[email protected]"

        Simulate.change(emailTextBox)

        const passwordTextBox = getByTestId("passwordTextBox")

        passwordTextBox.value = "fakePassword"

        Simulate.change(passwordTextBox)


        const loginForm = getByTestId("loginButton")

        Simulate.submit(loginForm)

        // Wait for tick after loginMuationMock
        await wait(() => {
          loginMuationMock.verify()
          expect(getByTestId("search-page")).toBeTruthy()
        })
      })
    })
  })
})

RyanAtViceSoftware added a commit to RyanAtViceSoftware/react-testing-library that referenced this issue May 7, 2018
@kentcdodds
Copy link
Member

🎉 This issue has been resolved in version 3.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

julienw pushed a commit to julienw/react-testing-library that referenced this issue Dec 20, 2018
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 a pull request may close this issue.

2 participants