Skip to content

Screen Export: Should it include the debug method? #417

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
jkdowdle opened this issue Dec 13, 2019 · 13 comments
Closed

Screen Export: Should it include the debug method? #417

jkdowdle opened this issue Dec 13, 2019 · 13 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@jkdowdle
Copy link
Contributor

jkdowdle commented Dec 13, 2019

Describe the feature you'd like:

I was just trying out the screen object to power the queries of my tests and the first thing I did was try using the debug method, expecting it to be there like from the object you get back from render in
'@testing-library/react'.

import React from 'react'
import { screen } from '@testing-library/react'

test('works', () => {
    setup()

    screen.debug()
})

And I noticed that it was not available. I would love to submit a PR to add it, but before I went looking around on how to do that I thought I would double check and make sure that that was not a conscious decision to leave out. Maybe some constraints on the difficulty of adding it or something like that.

Suggested implementation:

Describe alternatives you've considered:

We can continue getting the debug method off the returned object from rendering.

Or, as was mentioned bellow, use the logDOM export.

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

logDOM() // logs out document.body

Teachability, Documentation, Adoption, Migration Strategy:

This would increase the surface area by a little bit, but since screen was already added, it would not be that much more to add. I think users, like myself, would expect it to be there.

I want to thank all the maintainers, keep up the great work!

@kentcdodds
Copy link
Member

Hi @jkdowdle,

It's already supported:

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

logDOM() // logs out document.body

Though I can see the value of having it on screen so you can call/remove it without having to update imports.

Thoughts from anyone else?

@balazsorban44
Copy link

I did not think of it before, but now I am sure I would unconsciously go and try to get debug from screen every time! 😅

@kentcdodds
Copy link
Member

I'm not opposed to adding it, but I'd like to hear arguments for/against.

@alexkrolick
Copy link
Collaborator

Enzyme has a debug method for nodes, might make it easier to migrate nog screen did too

@benmonro
Copy link
Member

I vote Yes as this would make it a lot easier to report on debug in the testing lint rule. Actually for that matter why not Make it a top level export? I'd even support a breaking change to remove it from the results as that makes it quite tricky to lint when used with Custom render functions.

For note, the goal of the no debug lint rule is to keep it out of CI, and only use it locally temporarily when Debugging

@jkdowdle
Copy link
Contributor Author

jkdowdle commented Dec 16, 2019

@benmonro That sounds like the best argument so far for only allowing the top level export for the debug method, if I understood correctly. I personally haven't used the eslint rules yet for @testing-library but I'd like to start. Thank you

@benmonro
Copy link
Member

@jkdowdle there's also the jest-Dom lint plugin that I wrote. 😎

@ilovett
Copy link

ilovett commented Dec 27, 2019

I assumed based on experience with debug method returned from render that I could either do screen.debug or perhaps debug(screen) -- perhaps logDOM just needs to be better documented here https://testing-library.com/docs/dom-testing-library/api-helpers#debugging

Personally I like the syntax of debug(screen) ^_^ or debug(container) and I think it would maintain a consistent API

import { debug } from "@testing-library"

render();
debug(screen);

@kentcdodds
Copy link
Member

The benefit of putting it on screen rather than just having an extra export is that you don't have to update the imports to use debug. You can just type screen.debug() and then you can delete it and not worry about imports.

I think I'm in favor of this. Anyone wanna PR it?

@kentcdodds kentcdodds added enhancement New feature or request help wanted Extra attention is needed labels Jan 21, 2020
@jkdowdle
Copy link
Contributor Author

@kentcdodds I would agree! and I'd love to try to do the PR

@kentcdodds
Copy link
Member

Go for it 👍

@jkdowdle
Copy link
Contributor Author

jkdowdle commented Jan 29, 2020

debug has been added to the the screen object.
#429 and released in 6.11.1 6.12.0 as noted below in @bencomeau's comment

@bencomeau
Copy link

debug has been added to the the screen object.
#429 and released in 6.11.1 https://github.com/testing-library/dom-testing-library/releases/tag/v6.11.1

Great job on this! Just noting to clarify for future readers that it seems it was released with v6.12.0. Thanks again for the work to implement!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

7 participants