feat: Print link to the Testing-Library playground when getBy* queries fail #852

mathiassoeholm · 2020-12-19T17:48:10Z

What:

Adds a link to the Testing-Library playground, when getBy* queries fail, as requested in #837.

Why:

It adds awareness of the playground, which might help in making a better query.

How:

Changes the default getElementError function.

Checklist:

Documentation added to the
docs site, a PR has been added: Add documentation for printPlaygroundLink configuration option testing-library-docs#726
Tests
Typescript definitions updated
Ready to merge. Maybe, see my comment below

codesandbox-ci · 2020-12-19T17:49:23Z

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 07aa3a1:

Sandbox	Source
react-testing-library-examples	Configuration

codecov · 2020-12-19T17:49:53Z

Codecov Report

Merging #852 (07aa3a1) into main (c273ed5) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #852   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        27    +1     
  Lines          966       977   +11     
  Branches       293       303   +10     
=========================================
+ Hits           966       977   +11

Flag	Coverage Δ
node-10.14.2	`100.00% <100.00%> (ø)`
node-12	`100.00% <100.00%> (ø)`
node-14	`100.00% <100.00%> (ø)`
node-15	`100.00% <100.00%> (ø)`
node-16	`100.00% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/config.ts	`100.00% <100.00%> (ø)`
src/playground-helper.ts	`100.00% <100.00%> (ø)`
src/screen.js	`100.00% <100.00%> (ø)`

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 c273ed5...07aa3a1. Read the comment docs.

mathiassoeholm · 2020-12-19T17:50:20Z

It appears that Prettier might have formatted the inline snapshots differently, I'm not sure why 🤔

mathiassoeholm · 2020-12-23T11:28:39Z

Before I will call this Ready to be merged, I have a few questions:

~~Is it okay that I remove part of a test, that was causing the type check to fail? 43e7464~~ (Fixed by fix: add number support for matches #856)
Should @types/lz-string be a dependency or a dev-dependency? I added it as dev dependency, because it doesn't provide any types to the public API.
~~Why do I get a @typescript-eslint/no-unsafe-call lint warning, when I've added the types?~~ Seems like this was a local issue in my editor, and it is gone now.

4. Does anyone have a better suggestion for a text than "Open this DOM in the Testing-Library Playground"

eps1lon

Sorry for taking so long.

src/__tests__/get-by-errors.js

src/__tests__/element-queries.js

eps1lon · 2021-01-12T14:07:40Z

src/playground-helper.ts

+}
+
+function getPlaygroundUrl(element: Element | null, logErrors = true) {
+  if (!element || !('innerHTML' in element)) {


Suggested change

if (!element || !('innerHTML' in element)) {

if (element === null) {

Should be sufficient looking at the type definitions.

The original if-statement in logTestingPlaygroundURL in screen.js look like this:

if (!element || !('innerHTML' in element))

So changing it will cause a behaviour change for that function. Are we okay with that?

Considering we now have the type-safety I'm ok with it. Unless the types are inaccurate.

Hmm.. 🤔
In screen.d.ts the type for the element parameter is Element | HTMLDocument. However HTMLDocument does not have an innerHTML property.

It's not really clear to me why logTestingPlaygroundURL accepts a HTMLDocument if it requires innerHTML at the same time.

Should I keep Element | null or is Element | HTMLDocument | null the correct type?
Perhaps the type definition in screen.d.ts needs to be updated.

Another question: do we even want the | null? From the type definition it sounds like null is a valid value, when in fact it's an erroneous value to give as parameter.
If we do not have | null though, we will have to remove the first check or shut up ESLint with:

// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition if (!element) {

Update:

I have decided to remove the | null, so we're not "advertising" that null can be used as a valid value.

I don't think HTMLDocument is a valid type to use here, so to me it looks like that was an error in the old type definition.

I have removed a test that tried using {} as an argument and expecting The element you're providing isn't a valid DOM element. That error will no longer be logged if you're passing {}, because I removed the !('innerHTML' in element) check, and rely on static type checking instead.

That error will no longer be logged if you're passing {}, because I removed the !('innerHTML' in element) check, and rely on static type checking instead.

We generally don't relly on static type checking for users of our library. We may change that in the future but for now additional runtime errors should not be removed.

But these errors should happen in public methods. If getPlaygroundUrl is internal only then we can rely on static type checking and error should be thrown in the public methods using getPlaygroundUrl.

Thanks for the explanation, I think I get it now! I have added the runtime check again in logTestingPlaygroundURL.

src/playground-helper.ts

mathiassoeholm · 2021-01-13T19:41:33Z

Sorry for taking so long.

No worries! This is hardly an urgent issue, so thanks for taking the time to review :-)

src/__tests__/get-by-errors.js

src/config.ts

src/playground-helper.ts

src/config.ts

…ibrary#837

… a valid type.

Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>

src/__tests__/screen.js

types/screen.d.ts

eps1lon · 2021-06-02T11:56:19Z

src/playground-helper.ts

+function getPlaygroundUrl(element: Element) {
+  // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
+  if (!element) {


Suggested change

function getPlaygroundUrl(element: Element) {

// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition

if (!element) {

function getPlaygroundUrl(element: Element | null) {

if (element === null) {

Let's match runtime and static types

I touched upon this a little bit: #852 (comment)
Do you still think we should match the types, given that null is an erroneous type/value to pass in?

See #852 (comment)

internal methods: static type checking
public methods: static type checking + extraneous runtime type checking for users not using static type checking

Since the runtime checks has been moved to logTestingPlaygroundURL I was able to remove the if (element === null) from here :-)

mathiassoeholm · 2021-06-14T19:09:53Z

@eps1lon I think I finally understood the difference between error handling in internal and public facing functions, so the PR should be ready again now :-)

eps1lon

@eps1lon I think I finally understood the difference between error handling in internal and public facing functions, so the PR should be ready again now :-)

Mostly because I was side-lining this PR and didn't put much time into explaining what I think this PR should look like. Thank your for sticking with it 👍

I'll do some git hygene with the snapshot diffs, ensure compatibility with the alpha and then try it out in Material-UI to check if the playground link is not too noisy.

eps1lon

On second thought: Are we sure this doesn't affect findBy* perf? getMissingError for Byrole has a special branch when called in findBy*. We need to make sure performance of findBy isn't affected by this change.

golergka · 2023-05-29T12:52:16Z

What's the status of this issue? I want something like this implemented and would love to help.

mathiassoeholm mentioned this pull request Dec 23, 2020

Add documentation for printPlaygroundLink configuration option testing-library/testing-library-docs#726

Open

mathiassoeholm force-pushed the pr/playground-link-in-error-message branch from 34c9548 to f62b3a9 Compare December 23, 2020 11:19

mathiassoeholm changed the title ~~Pr/playground link in error message~~ Add link to the Testing-Library playground, when getBy* queries fail Dec 23, 2020

mathiassoeholm changed the title ~~Add link to the Testing-Library playground, when getBy* queries fail~~ Print link to the Testing-Library playground, when getBy* queries fail Dec 23, 2020

mathiassoeholm marked this pull request as ready for review December 23, 2020 11:36

mathiassoeholm force-pushed the pr/playground-link-in-error-message branch from f62b3a9 to 1f556b1 Compare December 23, 2020 11:54

mathiassoeholm force-pushed the pr/playground-link-in-error-message branch from f2371c3 to b567f10 Compare January 4, 2021 20:00

eps1lon requested changes Jan 12, 2021

View reviewed changes

mathiassoeholm force-pushed the pr/playground-link-in-error-message branch from e938cd9 to bb23329 Compare January 24, 2021 15:59

eps1lon reviewed Jan 25, 2021

View reviewed changes

src/__tests__/get-by-errors.js Show resolved Hide resolved

mathiassoeholm force-pushed the pr/playground-link-in-error-message branch from 4f71920 to 1c4f22b Compare January 27, 2021 17:41

eps1lon self-requested a review January 27, 2021 20:25

fildon reviewed May 9, 2021

View reviewed changes

src/config.ts Show resolved Hide resolved

eps1lon requested changes May 10, 2021

View reviewed changes

src/playground-helper.ts Outdated Show resolved Hide resolved

src/config.ts Outdated Show resolved Hide resolved

mathiassoeholm added 10 commits May 12, 2021 22:02

Remove test that makes type checking fail

6781d1d

Print a link to the playground when queries fail, resolved: testing-l…

03e80a1

…ibrary#837

Revert removal of type test

d7eae39

Make snapshot of error instead of partial match

aea9a16

Set printPlaygroundLink to false in global setup

00fb701

Throw error instead of logging

932d6bf

Fix function not being mocked because of import

568a330

Remove innerHTML check and rely on types instead. Also remove null as…

987fcbf

… a valid type.

Remove HTMLDocument as valid type for logTestingPlaygroundURL

54614d7

Clean up in afterEach instead of beforeEach

0b3d36e

mathiassoeholm force-pushed the pr/playground-link-in-error-message branch from cabd517 to 0b3d36e Compare May 12, 2021 20:02

mathiassoeholm and others added 2 commits May 12, 2021 22:03

Update src/config.ts

3be57c3

Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>

Update snapshot

4cfb5c9

eps1lon changed the title ~~Print link to the Testing-Library playground, when getBy* queries fail~~ feat: Print link to the Testing-Library playground when getBy* queries fail Jun 2, 2021

eps1lon added the enhancement New feature or request label Jun 2, 2021

eps1lon requested changes Jun 2, 2021

View reviewed changes

Add runtime check back in logTestingPlaygroundUrl

07aa3a1

eps1lon approved these changes Jun 14, 2021

View reviewed changes

eps1lon requested changes Aug 9, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Print link to the Testing-Library playground when getBy* queries fail #852

feat: Print link to the Testing-Library playground when getBy* queries fail #852

mathiassoeholm commented Dec 19, 2020 •

edited

codesandbox-ci bot commented Dec 19, 2020 •

edited

codecov bot commented Dec 19, 2020 •

edited

mathiassoeholm commented Dec 19, 2020

mathiassoeholm commented Dec 23, 2020 •

edited

eps1lon left a comment

eps1lon Jan 12, 2021

mathiassoeholm Jan 14, 2021

eps1lon Jan 14, 2021

mathiassoeholm Jan 14, 2021

mathiassoeholm Jan 14, 2021

mathiassoeholm Jan 24, 2021

eps1lon Jun 7, 2021 •

edited

mathiassoeholm Jun 14, 2021

mathiassoeholm commented Jan 13, 2021

eps1lon Jun 2, 2021

mathiassoeholm Jun 6, 2021

eps1lon Jun 7, 2021

mathiassoeholm Jun 14, 2021

mathiassoeholm commented Jun 14, 2021

eps1lon left a comment

eps1lon left a comment

golergka commented May 29, 2023

	if (!element \|\| !('innerHTML' in element)) {
	if (element === null) {

feat: Print link to the Testing-Library playground when getBy* queries fail #852

Are you sure you want to change the base?

feat: Print link to the Testing-Library playground when getBy* queries fail #852

Conversation

mathiassoeholm commented Dec 19, 2020 • edited

codesandbox-ci bot commented Dec 19, 2020 • edited

codecov bot commented Dec 19, 2020 • edited

Codecov Report

mathiassoeholm commented Dec 19, 2020

mathiassoeholm commented Dec 23, 2020 • edited

eps1lon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Update:

eps1lon Jun 7, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathiassoeholm commented Jan 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathiassoeholm commented Jun 14, 2021

eps1lon left a comment

Choose a reason for hiding this comment

eps1lon left a comment

Choose a reason for hiding this comment

golergka commented May 29, 2023

mathiassoeholm commented Dec 19, 2020 •

edited

codesandbox-ci bot commented Dec 19, 2020 •

edited

codecov bot commented Dec 19, 2020 •

edited

mathiassoeholm commented Dec 23, 2020 •

edited

eps1lon Jun 7, 2021 •

edited