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

feat(screen): Add screen export which has all queries bound to the body #412

Merged
merged 1 commit into from
Dec 13, 2019

Conversation

kentcdodds
Copy link
Member

@kentcdodds kentcdodds commented Dec 8, 2019

What: Add page export which has all queries bound to the body

Why: Drastically simplifies using queries in tests

How: Added a page.js file (and test) which will call getQueriesForElement(document.body) if there's a document.body globally available (if there's not, then folks will have to create their own queries which is fine because people without a global document are used to doing a lot of manual work. I still don't understand why they do this...)

Checklist:

This needs more discussion, but let me be clear... Despite this being a tiny PR, this is a huge deal. Observe:

Before:

import {render} from '@testing-library/react'

test('test 1', () => {
  const {queryByLabelText, getByText, getByLabelText} = render(<Thing />)
  // the rest of the test that uses the render queries
  // which happen to be bound to document.body by default anyway
  // for example:
  expect(queryByLabelText(/not here/i)).toBeNull()
})

test('test 2', () => {
  const {getByLabelText, queryByLabelText} = render(<Thing />)
  // the rest of the test that uses the render queries
  // which happen to be bound to document.body by default anyway
})

test('test 3', () => {
  const {getByLabelText, queryByLabelText} = render(<Thing />)
  // the rest of the test that uses the render queries
  // which happen to be bound to document.body by default anyway
})

test('test 4', () => {
  const {getByLabelText, queryByLabelText} = render(<Thing />)
  // the rest of the test that uses the render queries
  // which happen to be bound to document.body by default anyway
})

test('test 5', () => {
  const {getByLabelText, getByText} = render(<Thing />)
  // the rest of the test that uses the render queries
  // which happen to be bound to document.body by default anyway
})

test('test 6', () => {
  const {getByText} = render(<Thing />)
  // the rest of the test that uses the render queries
  // which happen to be bound to document.body by default anyway
})

test('test 7', () => {
  const {getByLabelText} = render(<Thing />)
  // the rest of the test that uses the render queries
  // which happen to be bound to document.body by default anyway
})

test('test 8', () => {
  const {getByLabelText, getByText} = render(<Thing />)
  // the rest of the test that uses the render queries
  // which happen to be bound to document.body by default anyway
})

After:

import {render, page} from '@testing-library/react'

test('test 1', () => {
  render(<Thing />)
  // the rest of the test that uses the page object
  // and people don't need to update destructuring
  // or even assign anything from render...
  // for example
  expect(page.queryByLabelText(/not here/i)).toBeNull()
})

test('test 2', () => {
  render(<Thing />)
  // the rest of the test that uses the page object
  // and people don't need to update destructuring
  // or even assign anything from render...
})

test('test 3', () => {
  render(<Thing />)
  // the rest of the test that uses the page object
  // and people don't need to update destructuring
  // or even assign anything from render...
})

test('test 4', () => {
  render(<Thing />)
  // the rest of the test that uses the page object
  // and people don't need to update destructuring
  // or even assign anything from render...
})

test('test 5', () => {
  render(<Thing />)
  // the rest of the test that uses the page object
  // and people don't need to update destructuring
  // or even assign anything from render...
})

test('test 6', () => {
  render(<Thing />)
  // the rest of the test that uses the page object
  // and people don't need to update destructuring
  // or even assign anything from render...
})

test('test 7', () => {
  render(<Thing />)
  // the rest of the test that uses the page object
  // and people don't need to update destructuring
  // or even assign anything from render...
})

test('test 8', () => {
  render(<Thing />)
  // the rest of the test that uses the page object
  // and people don't need to update destructuring
  // or even assign anything from render...
})

This is a much better experience IMO and it's what I think we should recommend in all our docs. A big benefit to this is that it's framework agnostic, so it will help avoid issues like testing-library/react-testing-library#538 (which has honestly always been a point of annoyance for me).

I'd love to hear what people think. I think we can merge this today, get the docs and TS stuff updated, then we can finally deal with the other PRs I've been neglecting.

@kentcdodds
Copy link
Member Author

Oh, and to be clear, I have no intention of removing existing functionality from render in React Testing Library. I think it's fine that we return the queries from that.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 8, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit fcc1e2b:

Sandbox Source
react-testing-library-examples Configuration

@kentcdodds
Copy link
Member Author

I would also consider calling it user instead of page. I'm willing to consider any other ideas as well.

@codecov
Copy link

codecov bot commented Dec 8, 2019

Codecov Report

Merging #412 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #412   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          21     22    +1     
  Lines         370    374    +4     
  Branches       86     87    +1     
=====================================
+ Hits          370    374    +4
Impacted Files Coverage Δ
src/screen.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60b65f5...fcc1e2b. Read the comment docs.

@balazsorban44
Copy link

balazsorban44 commented Dec 8, 2019

@kentcdodds Liked user better. In that case, maybe fireEvent methods could also be moved/copied under it, with third-person suffixes?

Eg.:
Before:

  • fireEvent.change()
  • fireEvent.click()

After:

  • user.changes()
  • user.clicks()

(or you could call it I! I.click(), I.getByLabelText() etc. 😅)

@benmonro
Copy link
Member

benmonro commented Dec 8, 2019

Just a thought but what about adding the queries to window.document? I've thought for a long time it'd be nice to have them built into the browsers living side by side with document.querySelector

@Georift
Copy link

Georift commented Dec 8, 2019

I really like the look of this!

Is there anything we could do to improve the error message in the environments where the global document.body is not present?

Importing page and getting null seems a little confusing. Or perhaps this isn't as much of a problem as I think, I'm still quite new to this library and don't fully understand when document.body wouldn't be present.

@benmonro
Copy link
Member

benmonro commented Dec 8, 2019 via email

@kentcdodds
Copy link
Member Author

Just a thought but what about adding the queries to window.document? I've thought for a long time it'd be nice to have them built into the browsers living side by side with document.querySelector

I feel like doing that would elevate querySelector to the same level as the rest of the queries (which we definitely don't want). Also, I don't like messing with global variables like that.

In that case, maybe fireEvent methods could also be moved/copied under it, with third-person suffixes?

We have user-event which does this kind of thing. I think it's interesting, but the problem that I see with doing this is that there are some events that don't have a corollary user-event method which means we'd need to still include fireEvent for folks and then you have people asking the question "which do I use." I guess they already do that as it is... Hmmm.... I'm uncertain.

In any case, if we were to add events to this user or page export, I think they should behave like user-event rather than fireEvent.

What about just exporting it from the root?

All the (unbound) queries are already exported from the root.

Also, part of the benefit that we get from a namespace like page is you don't have to keep updating the import when you need to add/remove methods you need.

@cloud-walker
Copy link

Just adding to the mix the fact that in react-native a page is called screen.

@weyert
Copy link
Contributor

weyert commented Dec 8, 2019

Personally, I really like this idea @kentcdodds. The suggestion made by earlier I am not sure whether it makes sense to use user for exposing these queries.

Maybe we could expose the user-event methods using that user in similar way as suggested here. I think that would make sense. I don't think a user would want to queryByRole for example.

@alexkrolick
Copy link
Collaborator

alexkrolick commented Dec 9, 2019

Added a page.js file
it's framework agnostic

Cool. I like that you can change to a new framework and just update the render line. Also guides you to think outside the framework when you write the test. No "wrapper objects".

Also, part of the benefit that we get from a namespace like page is you don't have to keep updating the import when you need to add/remove methods you need.

This is why I haven't been destructuring the render result in RTL. Intellisense on the page object is nice, you learn the queries as you go and can see all the possibilities right there.

In any case, if we were to add events to this user or page export, I think they should behave like user-event rather than fireEvent.

👍 yes, if we do that

Just adding to the mix the fact that in react-native a page is called screen.

🤔

I don't think a user would want to queryByRole for example.

page could keep fireEvent for lower-level uses and user would have the other actions. On the other hand... "finding elements" is an action as much as clicking and typing.

I have been using page to keep the name a bit more grounded on elements and query scope but I am not sure it's needed necessarily especially in the context of React Native using a different word.


import {page, user, render} from '@testing-library/react'

test('does x', () => {
  render(<Comp />)

  user.click(page.getByText('Sign In'))
  user.type(await page.findByLabelText('Username'), 'Kent')
  user.type(await page.findByLabelText('Password'), 'abc123')
  user.click(page.getByRole('button', {name: 'Submit'}))

  expect(await page.findByText('Welcome')).toBeInTheDocument()
})

@kentcdodds
Copy link
Member Author

I'd love to great what @Gpx has to say about including user-event as an export from DOM Testing Library (or maybe even moving it into this project completely).

@AlgusDark
Copy link

What is the difference of...?

const page = render(<Thing />)
// page.queryXXXXXXX
// page.getByXXXXX

@trevorwright

This comment has been minimized.

@alexkrolick
Copy link
Collaborator

@trevorwright @AlgusDark this is a feature of DOM Testing Library, not the React wrapper.

  • It simplifies the wrapper implementations
  • It would reduce boilerplate in each test and you don't have to think about the meaning or naming of the page/user object because it is persistent.

src/page.js Outdated Show resolved Hide resolved
eps1lon
eps1lon previously approved these changes Dec 9, 2019
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

This makes a lot more sense with the afterEach(cleanup) recommendation. One less footgun when dealing with component libraries and portals 👍

@weyert
Copy link
Contributor

weyert commented Dec 9, 2019

import {page, user, render} from '@testing-library/react'

test('does x', () => {
  render(<Comp />)

  user.click(page.getByText('Sign In'))
  user.type(await page.findByLabelText('Username'), 'Kent')
  user.type(await page.findByLabelText('Password'), 'abc123')
  user.click(page.getByRole('button', {name: 'Submit'}))

  expect(await page.findByText('Welcome')).toBeInTheDocument()
})

I quite like your bottom code snippet :) Makes sense to me!

@kentcdodds
Copy link
Member Author

with the afterEach(cleanup) recommendation

To be clear, people shouldn't need to do that because it's automatic.

I like your suggestion and I'll implement that 👍

@kentcdodds
Copy link
Member Author

I think this should be merged and we can start updating docs etc. Then we can discuss the user export in another PR/issue.

@kentcdodds
Copy link
Member Author

Actually...

Just adding to the mix the fact that in react-native a page is called screen.

Should we call it screen as well? Keep it consistent? What do you think @bcarroll22? I'm fine with calling it a screen for web.

src/__node_tests__/page.js Outdated Show resolved Hide resolved
src/page.js Outdated Show resolved Hide resolved
@eps1lon
Copy link
Member

eps1lon commented Dec 11, 2019

@kentcdodds In case this wasn't by accident: You can batch suggestions into a single commit. You need to switch to the "Files changed" tab though.

@afontcu
Copy link
Member

afontcu commented Dec 11, 2019

This looks like a good improvement. Totally on board 👍

Regarding the user-event debate, should we open a new issue and discuss it there? From my point of view user-event should end up replacing fireEvent… eventually (we would be getting "closer" to the user).

@bcarroll22
Copy link

Sorry it took me a couple days to get to this, holidays have things crazy 😬

I'm cool with working on getting screen as a top-level export in react-native. I think screen is a good name for it; pages aren't a thing for RN users but they're generic enough that they can make sense to DOM user.

As far as the user-event stuff goes, if that one becomes a top level export, we're almost just have to rename fireEvent to user because we don't really have higher-level actions like user-event exposes.

So I'm good on the screen export, and the other part for us would essentially be changing fireEvent to user if that's the direction we go 👍

@kentcdodds
Copy link
Member Author

Let's open a new issue to discuss the adoption of user as an export for event firing.

I'm going to make a PR for docs before I merge this. If anyone would like to add a PR for the TypeScript typings that would be super duper :)

@kentcdodds kentcdodds changed the title feat(page): Add page export which has all queries bound to the body feat(screen): Add page export which has all queries bound to the body Dec 13, 2019
@kentcdodds kentcdodds merged commit 4fed5ae into master Dec 13, 2019
@kentcdodds kentcdodds deleted the pr/add-page branch December 13, 2019 15:26
@kentcdodds
Copy link
Member Author

🎉 This PR is included in version 6.11.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@bowernite
Copy link

@kentcdodds Curious on your thought on how this plays with render "factories" that previously piggy backed on the utils exported from render.

Previous:

test('test 1', () => {
  const { usernameInput, passwordInput, getByLabelText } = renderComponent()
  ...
}

function renderComponent() {
  const utils = render(<MyComponent />)
  return {
    ...utils,
    usernameInput: utils.getByLabelText(/username/i),
    passwordInput: utils.getByLabelText(/password/i),
  }
}

Ideally, to fully lean in to the experience benefit of not updating the destructuring, we wouldn't return keys for elements/utils like usernameInput or passwordInput anymore, but not sure of an alternative.

We could just tack on to the screen "global", but that's a bit hacky, and you lose out on intellisense.

@trevorwright
Copy link

@babramczyk Our team uses this pattern a lot, especially when you are writing tests for larger more complex features. It allows for simple management of selectors so that if the label changes from username to email you only have one piece of a test to update instead finding all the strings across tests.

It really helps when you write some sort of custom code for helping RTL easily interact with your feature, and allowing you to continually reuse that code. One thing we will do in larger features is also use this pattern to namespace the features a bit, for example (somewhat simplified)

const renderComponent = () => {
  const utils = render(<MyComponent />)

  const setSelectedDate = () => {}
  const selectedDateValue = () => {}
  const setFilter = () => {}

  return {
    ...utils,
    header: {
      setSelectedDate,
      selectedDateValue,
      setFilter,
    }
  }
}

// later in tests
const { header } = renderComponent();
header.setSelectedDate(new Date());
expect(header.selectedDateValue()).toBe('Today');

@kentcdodds
Copy link
Member Author

Yeah, I think it's fine to stick with that pattern. I definitely wouldn't recommend monkey-patching on to screen.

@kentcdodds kentcdodds restored the pr/add-page branch March 12, 2020 21:31
@kentcdodds kentcdodds deleted the pr/add-page branch March 12, 2020 21:53
@hitmands
Copy link

@Agentkma
Copy link

Hey @kentcdodds i'm trying to use this setup and i keep getting 'page' as undefined when I try to import it...either from my utils file or directly from @testing-library/react ....trying to use this b/c i have customRender setup based on RTL docs for redux. Thanks for any direction!

@kentcdodds
Copy link
Member Author

It ended up getting called screen rather than page :)

@eps1lon eps1lon changed the title feat(screen): Add page export which has all queries bound to the body feat(screen): Add screen export which has all queries bound to the body Aug 12, 2020
@Agentkma
Copy link

It ended up getting called screen rather than page :)

ok, cool, yes I was using screen before and was getting jest error TypeError: matcher.test is not a function at my line const monday = screen.getByLabelText(LABEL_1);... which sent me searching and finding this issue...this started happening when i created the renderWithRedux func in the test util files per RTL docs....thanks for clearing the page thing up :)

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

Successfully merging this pull request may close these issues.