feat: suggestions for which query to use #586

benmonro · 2020-05-25T07:29:46Z

What:
Adding suggestions for which query to use. (opt in via configure). When using configure({throwSuggestions: true}) tests will fail if they are not using the suggested queries. While this may seem harsh, the intent is more along the lines of the way that --detectLeaks works in jest.

Why:
To encourage people to follow best practices when using TL.

How:
a new getSuggestedQuery function that follows the order of 'which query should i use?'

Started going down the road of having a separate library but it became apparent pretty quick that it made more sense for this to live in DTL directly. We can export the suggestion from here for things like testing-playground.com

Docs PR: https://github.com/testing-library/testing-library-docs/pull/477/files

Checklist:

Support all use cases of 'which query should i use'
Documentation added to the
docs site
I've prepared a PR for types targeting
DefinitelyTyped
Tests
Ready to be merged

codesandbox-ci · 2020-05-25T07:30:45Z

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 9cec773:

Sandbox	Source
little-surf-usbk4	Configuration

codecov · 2020-05-25T07:51:38Z

Codecov Report

Merging #586 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #586   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        24    +1     
  Lines          468       535   +67     
  Branches       115       133   +18     
=========================================
+ Hits           468       535   +67

Impacted Files	Coverage Δ
src/config.js	`100.00% <ø> (ø)`
src/wait-for.js	`100.00% <ø> (ø)`
src/queries/alt-text.js	`100.00% <100.00%> (ø)`
src/queries/display-value.js	`100.00% <100.00%> (ø)`
src/queries/label-text.js	`100.00% <100.00%> (ø)`
src/queries/placeholder-text.js	`100.00% <100.00%> (ø)`
src/queries/role.js	`100.00% <100.00%> (ø)`
src/queries/test-id.js	`100.00% <100.00%> (ø)`
src/queries/text.js	`100.00% <100.00%> (ø)`
src/queries/title.js	`100.00% <100.00%> (ø)`
... and 3 more

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 709044b...9cec773. Read the comment docs.

benmonro · 2020-05-25T13:37:21Z

src/query-helpers.js

+        suggestions.length === 1 &&
+        !allQuery.name.endsWith(rawSuggestions[0].queryName)
+      ) {
+        throw getSuggestionError(suggestions[0], container)


@kentcdodds was pairing on this with my friend Kevin and he asked a good question. Should the suggestion Try to actually execute the query it's suggesting just to make sure it works? Curious to hear your thoughts.
..

benmonro · 2020-05-25T22:40:27Z

src/queries/placeholder-text.js

-
+function queryAllByPlaceholderText(...args) {
+  return queryAllByAttribute('placeholder', ...args)
+}


ICYW: all of the other queries use named functions and the suggestion feature needs them to be named.

benmonro · 2020-05-25T22:40:46Z

src/queries/test-id.js

-  queryAllByAttribute(getTestIdAttribute(), ...args)
+function queryAllByTestId(...args) {
+  return queryAllByAttribute(getTestIdAttribute(), ...args)
+}


ICYW: all of the other queries use named functions and the suggestion feature needs them to be named.

src/query-helpers.js

smeijer · 2020-05-26T14:29:37Z

Copy from Twitter again 😇

@benmonro
We've mentioned something like "screen.getByLabelText('foo', { silent: true })" and agreed it's not the best solution.

How do you feel about using an "instance" level for it instead? screen.showSuggestions(true|false), where you can run queries in between:

screen.showSuggestions(false);

document.querySelector('input'); // don't throw this time

screen.showSuggestions(true);

Or an alternative, a wrapping method like within

Name it strict?

const { getByText } = strict(document.getElementById('messages'));
const helloMessage = getByText('hello')

This could then be accompanied by a screen.strict = true, which enables strict mode for all screen.x queries.

benmonro · 2020-05-26T14:50:20Z

@kentcdodds @eps1lon @smeijer I tried using warn (screenshot below). I'm definitely not a fan for 2 reasons:

you don't really know which test it's in. Failing the test will make it very obvious which test is using the query we're suggesting for improvement. If you know of a way to tie the warning to the line and/or test that's using the query I could be open to that, but otherwise how will people know where the suggestion is pointing?
While the difference between getByLabel and getByRole might be slight, my biggest goal with this is to get people to stop using getByTestId which in my view does in fact warrant a fail when there's something better out there.

smeijer · 2020-05-26T14:58:16Z

While the difference between getByLabel and getByRole might be slight, my biggest goal with this is to get people to stop using getByTestId which in my view does in fact warrant a fail when there's something better out there.

Do I understand correctly that you only throw for getByTestId queries? Because if throwing only happens for querySelector and getByTestId when the dev chose to opt-in, then I (as a developer) would have zero issues with throwing.

Heck, I can imagine something like that being default in a future version.

benmonro · 2020-05-26T15:13:09Z

Do I understand correctly that you only throw for getByTestId queries?

no, I'm not currently having this warn on usages of querySelector, that would be a bit sketchy imo as we'd have to change the way querySelector works. Also I feel like that one can be done w/ a lint rule. As for getByTestId, no I do not only throw on that. It throws whenever a better option is available.

If we use warn we'd have to parse the stack trace (not sure when I'd have cycles to do that) have a look at my screenshot above (or feel free to pull my branch and play with it). If you can get the stack trace parsing working I'd be fine w/ having it use a warn (or maybe make that optional as well?). We can do new Error().stack but it would have to ultimately highlight the exact line in their test where the query was called or it's better to just fail the test imo.

I'll lay out the options here (in no particular order) for how to disable the suggestion for a single query, feel free to add more options, but maybe we can take a vote?

option 1: an additional query option in the query itself:

screen.getByTestId("foo", {suggest: false})

option 2: just use configure:

configure({showSuggestions:false});
screen.getByTestId("foo")
configure({showSuggestions:true});

option 3: Stephan's screen.showSuggestions idea

screen.showSuggestions(false);

document.querySelector('input'); // don't throw this time

screen.showSuggestions(true);

option 4: more granular configuration option for showSuggestions

configure({
  showSuggestions: { ByLabelText: "warn", ByTestId: "error" }
})

smeijer · 2020-05-26T16:14:18Z

Option 5

// strict || strictA11y || ...

const { getByText } = strict(document.getElementById('messages'));
const helloMessage = getByText('hello')

A method like that fits nicely together with within in terms of code style.

Talking about within. If I'm not mistaken, that method also modifies the error stack to print the correct code frame. That's why we have the option showOriginalStackTrace.

I do agree, that is a must for this to work nicely. Showing incorrect code frames is nothing more than confusing.

benmonro · 2020-05-26T16:22:17Z

@smeijer ah ok i'll look into how that prints the stack trace.

w/ regards to using strict(document.querySelector|getElementById) not sure I follow the point there... imo if you are using any of the primitive options to query the DOM that can be handled by a lint rule. can you elaborate on how you see that working? it's not clear to me...

smeijer · 2020-05-26T16:53:20Z

edit Figured it out, and deleted some background info, you can still see it under the "edits".

What I'm proposing is an API like this:

import { screen, within } from '@testing-library/dom';

const view = within(document.getElementById('messages'), { strict: true });
view.getByText('hello'); // might throw for a11y issues

screen.getByText('hello'); // still not throwing

This would play nicely together with your option 1.

import { screen, getByText } from '@testing-library/dom';

screen.getByText('hello', { strict: true }); // might throw
getByText(document, 'hello', { strict: true }); // might throw

ps. I'm deliberately naming it strict and not suggest. As throwing errors isn't suggesting. It's enforcing. Naming it something like strictA11y would also be fine.

benmonro · 2020-05-29T15:55:58Z

@kentcdodds @smeijer all the feedback has been addressed. docs PR is good to merge as well as soon as this merges.

timdeschryver

Really looking forward to this 👍
Should the extra config suggest also be added to the matcher types?

kentcdodds · 2020-05-29T17:38:06Z

Yes 👍

benmonro · 2020-05-29T18:16:28Z

@timdeschryver @kentcdodds added the type to matcher types.

…-testing-library into feature/suggestions

types/matches.d.ts

kentcdodds

Stellar! Thanks!

kentcdodds · 2020-05-29T18:52:21Z

🎉 This PR is included in version 7.7.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

benmonro · 2020-05-29T18:52:32Z

@allcontributors please add @appleJax for code & tests

allcontributors · 2020-05-29T18:52:42Z

@benmonro

I've put up a pull request to add @appleJax! 🎉

benmonro · 2020-05-29T19:06:32Z

@allcontributors please add @benmonro for code, ideas, tests, docs

allcontributors · 2020-05-29T19:06:42Z

@benmonro

I've put up a pull request to add @benmonro! 🎉

smeijer · 2020-05-29T19:08:57Z

🎉

Nice work @benmonro ! Thanks.

smeijer · 2020-05-29T19:41:35Z

I don't think that this pull-request would have been here if it wasn't for http://testing-playground.com, so how about this one as well?

@allcontributors please add @smeijer for ideas

allcontributors · 2020-05-29T19:41:46Z

@smeijer

I've put up a pull request to add @smeijer! 🎉

benmonro · 2020-05-29T19:43:31Z

@smeijer yep yep

feat: suggestions for which query to use

0cf9fd9

benmonro added enhancement New feature or request needs discussion We need to discuss this to come up with a good solution needs investigation labels May 25, 2020

coverage bumped

7cb52ba

benmonro commented May 25, 2020

View reviewed changes

benmonro and others added 5 commits May 25, 2020 11:18

Merge branch 'master' into feature/suggestions

e0b7f69

more tests, label text working

75c492c

more cases for labelText

51c377c

removed commented out code

be480ad

added *ByDisplayValue

63b26cb

benmonro commented May 25, 2020

View reviewed changes

Ben Monro added 4 commits May 25, 2020 22:27

all queries supported now

a0dc8f9

cleanup

5ee9649

added types for suggestions

625ce15

export suggestions from index

1db13e6

benmonro requested a review from kentcdodds May 26, 2020 06:01

fixed a couple lint warnings

30e64e2

smeijer reviewed May 26, 2020

View reviewed changes

src/query-helpers.js Outdated Show resolved Hide resolved

benmonro removed the needs discussion We need to discuss this to come up with a good solution label May 27, 2020

benmonro added 6 commits May 29, 2020 08:01

suggest on labeltext

18b1813

suggest on queryAllBy

2a741e9

more tests

ac9e9df

rename showSuggs to throwSuggs

53460df

PR feedback

fa69912

Merge branch 'master' into feature/suggestions

8861c24

timdeschryver reviewed May 29, 2020

View reviewed changes

matches.d.ts

7ec0b97

Merge branch 'feature/suggestions' of https://github.com/benmonro/dom…

51618bd

…-testing-library into feature/suggestions

kentcdodds reviewed May 29, 2020

View reviewed changes

types/matches.d.ts Outdated Show resolved Hide resolved

Update types/matches.d.ts

9cec773

kentcdodds approved these changes May 29, 2020

View reviewed changes

kentcdodds merged commit 839dca3 into testing-library:master May 29, 2020

benmonro deleted the feature/suggestions branch May 29, 2020 18:49

kentcdodds added the released label May 29, 2020

allcontributors bot mentioned this pull request May 29, 2020

docs: add appleJax as a contributor #594

Merged

allcontributors bot mentioned this pull request May 29, 2020

docs: add benmonro as a contributor #595

Merged

allcontributors bot mentioned this pull request May 29, 2020

docs: add smeijer as a contributor #596

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: suggestions for which query to use #586

feat: suggestions for which query to use #586

benmonro commented May 25, 2020 •

edited

codesandbox-ci bot commented May 25, 2020 •

edited

codecov bot commented May 25, 2020 •

edited

benmonro May 25, 2020

benmonro May 25, 2020

benmonro May 25, 2020

smeijer commented May 26, 2020

benmonro commented May 26, 2020 •

edited

smeijer commented May 26, 2020

benmonro commented May 26, 2020

smeijer commented May 26, 2020

benmonro commented May 26, 2020

smeijer commented May 26, 2020 •

edited

benmonro commented May 29, 2020

timdeschryver left a comment

kentcdodds commented May 29, 2020

benmonro commented May 29, 2020

kentcdodds left a comment

kentcdodds commented May 29, 2020

benmonro commented May 29, 2020

allcontributors bot commented May 29, 2020

benmonro commented May 29, 2020

allcontributors bot commented May 29, 2020

smeijer commented May 29, 2020 •

edited

smeijer commented May 29, 2020

allcontributors bot commented May 29, 2020

benmonro commented May 29, 2020

feat: suggestions for which query to use #586

feat: suggestions for which query to use #586

Conversation

benmonro commented May 25, 2020 • edited

codesandbox-ci bot commented May 25, 2020 • edited

codecov bot commented May 25, 2020 • edited

Codecov Report

benmonro May 25, 2020

Choose a reason for hiding this comment

benmonro May 25, 2020

Choose a reason for hiding this comment

benmonro May 25, 2020

Choose a reason for hiding this comment

smeijer commented May 26, 2020

benmonro commented May 26, 2020 • edited

smeijer commented May 26, 2020

benmonro commented May 26, 2020

smeijer commented May 26, 2020

benmonro commented May 26, 2020

smeijer commented May 26, 2020 • edited

benmonro commented May 29, 2020

timdeschryver left a comment

Choose a reason for hiding this comment

kentcdodds commented May 29, 2020

benmonro commented May 29, 2020

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented May 29, 2020

benmonro commented May 29, 2020

allcontributors bot commented May 29, 2020

benmonro commented May 29, 2020

allcontributors bot commented May 29, 2020

smeijer commented May 29, 2020 • edited

smeijer commented May 29, 2020

allcontributors bot commented May 29, 2020

benmonro commented May 29, 2020

benmonro commented May 25, 2020 •

edited

codesandbox-ci bot commented May 25, 2020 •

edited

codecov bot commented May 25, 2020 •

edited

benmonro commented May 26, 2020 •

edited

smeijer commented May 26, 2020 •

edited

smeijer commented May 29, 2020 •

edited