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

Queries for tables #583

Open
lexanth opened this issue May 19, 2020 · 23 comments
Open

Queries for tables #583

lexanth opened this issue May 19, 2020 · 23 comments
Labels
needs discussion We need to discuss this to come up with a good solution

Comments

@lexanth
Copy link

lexanth commented May 19, 2020

Describe the feature you'd like:

The current queries don't really allow anything matching how a user would interact with a table.
There is limited support using the ByRole selectors, to choose all rows, or all cells, or nesting between these, but doesn't always match the principle of "use your component as a user would".

e.g. In a table where each row has a "Delete" icon, choosing the delete icon corresponding to the particular row contents. A user would identify which row they're looking at by the value in one of the cells, then scanning across.

Suggested implementation:

I've implemented some custom queries in https://github.com/lexanth/testing-library-table-queries#readme, but some of it is a bit hacky.

Describe alternatives you've considered:

getAllByRole('row') gets rows, but then filtering by text content is slightly awkward (either index based or filtering based on content).

Teachability, Documentation, Adoption, Migration Strategy:

This would be additional queries, so backwards-compatible.

@kentcdodds kentcdodds added the needs discussion We need to discuss this to come up with a good solution label May 19, 2020
@eps1lon
Copy link
Member

eps1lon commented May 19, 2020

Could you post the markup for your table so that we can identify how testing-library should work?

@lexanth
Copy link
Author

lexanth commented May 28, 2020

As an example:

Name Age Actions
John 23 [Delete]
Jane 34 [Delete]
Joe 45 [Delete]
<table>
  <thead>
    <tr>
      <th>Name</th>
      <th>Age</th>
      <th>Actions</th>
    </tr>
  </thead>
  <tbody>
    <tr>
      <td>John</td>
      <td>23</td>
      <td><button>Delete</button></td>
    </tr>
    <tr>
      <td>Jane</td>
      <td>34</td>
      <td><button>Delete</button></td>
    </tr>
    <tr>
      <td>Joe</td>
      <td>45</td>
      <td><button>Delete</button></td>
    </tr>
  </tbody>
</table>

Query examples:
e.g. Joe's age should be 45 (might be worth testing as DOB is stored, but age is presented)

expect(getAllByRole('row')[3]).toContainTextContent(/45/)

Picking the row by index doesn't seem to be testing as a user would. And I'm not really testing the Age column, so if I added a phone number and he happened to have 45 in that, my test no longer verifies the age.

Really what I want to do here is "Get row with Joe in the first column, Get cell in "Age" column"

e.g. Click the delete button against Jane

fireEvent.click(within(getAllByRole('row')[2]).getByText('Delete'))

Again, I'm having to pick the row by index, which makes the test more brittle and harder to refactor if something changes.

Another alternative would be adding data-testid attributes all over that table, but again, not exactly how a user would see it.

Really what I want to do here is "Get row with Jane in the first column, click item inside with Delete text"

@alexkrolick
Copy link
Collaborator

alexkrolick commented May 28, 2020

const row = getByRole('cell', {name: 'Jane'}).closest('tr')
fireEvent.click(
  within(row).getByText('Delete')
)

Of course, this doesn't work if you aren't using literal table elements, virtualized tables, etc.

@filipegiusti
Copy link

How about

const row = getByText('Jane', { closest: 'row' })
fireEvent.click(
  within(row).getByText('Delete')
)

or

const row = within(getByRole('cell', {name: 'Jane'})).getByClosestRole('row')
fireEvent.click(
  within(row).getByText('Delete')
)

The general idea is to use a role to get the closest element instead of a selector.

@jarek-jpa
Copy link

@filipegiusti I guess these are further api proposals, rather than actual (workaround) implementation?

@filipegiusti
Copy link

Exactly, those are further api proposals. As this is from May I don't expect it to be ever approved.

@nickserv
Copy link
Member

nickserv commented Nov 9, 2020

Instead of within couldn't we nest query function calls?

fireEvent.click(getByText(getBySomething(rowStuff), 'Delete'))

@bradleylandis
Copy link

Are there any examples or documentation of the current best practices for working with tables?

@joshua-phillips
Copy link

I'm also interested in this. I feel the same that testing table's right now does not feel like testing how a user would.

If I were to visually process the rows of data in a table, I would typically reference the column header text as I scan across the row data.

Alternatively, I might look for a particular column, and then scan down the table looking for values of interest.

As a user, you're also typically using context to determine what constitutes uniqueness of data in a table. Perhaps it's a single column value, but maybe it's a composite key of two or more columns like first and last name for instance.

So in that case, say I'm looking for the favorite color of someone named "Steve Rogers." I might scan for first name's of "Steve" and as I find a row that matches, then I'd check whether the value in the last name column matches "Rogers" continuing my scan until the desired row is found or no more rows exist. If found, I could look to the column for favorite color to get the information I'm looking for.

Expressed in RTL, I think the way I'd have to achieve that now would be something like this (untested and just typed here by hand so not sure this is 100% sound).

let favoriteColor = null;

const knownIndexOfColorColumn = getColorColumnIndex();
const table = screen.getByTitle("Super hero stats");

const firstNames = within(table).queryAllByText("Steve");

for (let i = 0; i < firstNames.length; i++) {
    const row = firstNames[i].parent;

    // perhaps this should even limit the search to within the specific cell of the last name column
    const lastName = within(row).queryByText("Rogers");

    if (lastName) {
        favoriteColor = within(row).getAllByRole("cell")[knownIndexOfColorColumn];

        break;
    }
}

expect(within(favoriteColor).getByText("Red")).toBeInTheDocument();

But that just doesn't seem very elegant or to be in the spirit of RTL. I don't have a proposal for how to do it in a more "user testing" manner, but maybe I'm just being rigid in my thinking and someone can offer a better way that the above could be achieved in a more RTL-ish way.

@ThomasGHenry
Copy link

This got my head in the right space to solve a similar issue.
https://polvara.me/posts/five-things-you-didnt-know-about-testing-library
Specifically, const row = screen.getByText(id).closest("tr");

@cbookg
Copy link

cbookg commented Nov 25, 2021

A couple of solutions here suggest using 'closest' but the testing-library linter dislikes this:
https://github.com/testing-library/eslint-plugin-testing-library/blob/main/docs/rules/no-node-access.md

@kealjones-wk
Copy link

kealjones-wk commented Dec 13, 2021

After thinking about this alot, I found a way that seems to work without using closest and would most "resemble the way your software is used".

getByRole('row', { name: /Joe/ })

testing-playground

You can then use within to query for other things in the row.

All rows get an aria accessible name that is all the text content within the row separated by spaces. The example table originally on this thread has the aria name Joe 45 Delete.

By using regex for the name option you are able to isolate the row you need both ByRole and indirectly "ByText".

@enagy27
Copy link

enagy27 commented Dec 19, 2021

I've been digging into this for a project I've been working on with the intention of building queries based on table headers, which is something I've found challenging in the past. I tried to be pretty comprehensive to account for semantic and aria-* tables alike with various rowSpans and colSpans.

I'm sure that I've made some incorrect assumptions but figured I'd share what I came up with and propose a path forward if this is of interest.

Code sample

Here's the gist of my solution 🔗

So far I've tested this for the following table:

Table of Items Sold August 2016 with nested row headers and column headers which is tested in the provided Github Gist

┌─────────┬───────────────────┬─────────────┬────────────┬───────────┬───────────┬───────────────┬───────────────┐
│ (index) │         0         │      1      │     2      │     3     │     4     │       5       │       6       │
├─────────┼───────────────────┼─────────────┼────────────┼───────────┼───────────┼───────────────┼───────────────┤
│    0    │        ''         │     ''      │ 'Clothes'  │ 'Clothes' │ 'Clothes' │ 'Accessories' │ 'Accessories' │
│    1    │        ''         │     ''      │ 'Trousers' │ 'Skirts'  │ 'Dresses' │  'Bracelets'  │    'Rings'    │
│    2    │     'Belgium'     │  'Antwerp'  │    '56'    │   '22'    │   '43'    │     '72'      │     '23'      │
│    3    │     'Belgium'     │   'Gent'    │    '46'    │   '18'    │   '50'    │     '61'      │     '15'      │
│    4    │     'Belgium'     │ 'Brussels'  │    '51'    │   '27'    │   '38'    │     '69'      │     '28'      │
│    5    │ 'The Netherlands' │ 'Amsterdam' │    '89'    │   '34'    │   '69'    │     '85'      │     '38'      │
│    6    │ 'The Netherlands' │  'Utrecht'  │    '80'    │   '12'    │   '43'    │     '36'      │     '19'      │
└─────────┴───────────────────┴─────────────┴────────────┴───────────┴───────────┴───────────────┴───────────────┘

API

My proposal is to add a query, byTableHeaderText, and a corresponding matcher, toHaveTableHeaderText to enable the following:

const gentRow = within(table).getByRole('row', /Gent/);

const gentTrousers = within(gentRow).getByTableHeaderText('Trousers');
expect(gentTrousers).toHaveTableHeaderText('Gent');

A few more examples:

// Note: the second argument with scope is optional
const belgiumCells = screen.getAllByTableHeaderText('Belgium', { scope: 'rowgroup' });
const gentCells = screen.getAllByTableHeaderText('Gent', { scope: 'row' });

const gentHeader = within(table).getByRole('rowheader', { name: 'Gent' });
expect(gentHeader).toHaveTableHeaderText('Belgium', { scope: 'rowgroup' });

const clothesCells = screen.getAllByTableHeaderText('Clothes', { scope: 'colgroup' });
const trousersCells = screen.getAllByTableHeaderText('Trousers', { scope: 'col' });

const trousersHeader = within(table).getByRole('columnheader', { name: 'Trousers' });
expect(trousersHeader).toHaveTableHeaderText('Clothes', { scope: 'colgroup' });

// Filter to get the intersection
const gentTrousersCells = _.intersection(gentCells, trousersCells);

// Alternatively without lodash this intersection would be:
// gentCells.filter(cell => trousersCells.includes(cell));

If this is of interest I'd be happy to put up a PR!

@jason-yang
Copy link

This is how I am doing it right now:

const row = screen.getAllByRole('row').find((row) => within(row).queryByText(text) !== null);

This means it will return undefined if no result is found and the HTMLElement if found, which still doesn't match up to exactly what get* means but it's better than nothing...

@anilanar
Copy link

I think we have a winner idea already, the most liked comment above.

@cjsilva-umich
Copy link

Was this ever integrated into testing-library?

@mathieu-suen-sonarsource

It is often that you would like to assert the content of a tabular information
To me a nice API would look like:

expect(screen.getByRole('cell', {row: 1, column: 1, name: 'foo'})).toBeInDocument();
expect(screen.getByRole('cell', {columnheader: 'foo', rowheader: 'bar', name: '1'})).toBeInDocument();

@Jhonatangiraldo
Copy link

Off the topic sorry, but why the role is cell and not gridcell as shown int he inspector ? @mathieu-suen-sonarsource

@mathieu-suen-sonarsource

cell is the more generic role of a gridcell according to w3c

@mathieu-suen-sonarsource

This let me think that we can also encourage the following techniques using this API:
expect(screen.getByRole('cell', {headers: ['a', 'b']})).toBeInDocument();

@christopherstock
Copy link

christopherstock commented Sep 4, 2024

why not using snapshot testing on the entire table in order to assert all table headers/rows/cells to be exact values? 😊

    const table = screen.queryAllByRole('table');
    expect(table).toMatchSnapshot();

@bl-nero
Copy link

bl-nero commented Sep 5, 2024

@christopherstock That's because snapshot testing doesn't apply universally. The moment you start applying it to non-trivial components, snapshot testing becomes a maintenance nightmare. In the context of unit testing this is generally a moot point; snapshot testing may have its place in the universe, but your argument effectively questions the entire idea of having query functions in the first place. That's why I think the need for querying the tables effectively still holds up.

@christopherstock
Copy link

@christopherstock That's because snapshot testing doesn't apply universally. The moment you start applying it to non-trivial components, snapshot testing becomes a maintenance nightmare. In the context of unit testing this is generally a moot point; snapshot testing may have its place in the universe, but your argument effectively questions the entire idea of having query functions in the first place. That's why I think the need for querying the tables effectively still holds up.

Thanks for the detailed explanation! @bl-nero 👍🏻 🌺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion We need to discuss this to come up with a good solution
Projects
None yet
Development

No branches or pull requests