Skip to content
This repository was archived by the owner on Jul 30, 2020. It is now read-only.

Remove styles object from debug() output #23

Closed
bcarroll22 opened this issue May 22, 2019 · 7 comments
Closed

Remove styles object from debug() output #23

bcarroll22 opened this issue May 22, 2019 · 7 comments
Labels
breaking change enhancement New feature or request help wanted Extra attention is needed

Comments

@bcarroll22
Copy link
Collaborator

Describe the feature you'd like:

Currently, in debug output, we filter out any prop that has a function as its value because it's noisy and not very helpful. I would like to propose that we also filter out styles because they have a tendency to be long and noisy, and generally not provide much value.

Suggested implementation:

I'd like to implement this by filtering these values out in debug output, but not snapshots. I don't see a reason to exclude the styles from snapshots, especially since in that context they could definitely be useful to ensure that your components are still being rendered as expected.

Describe alternatives you've considered:

I'd considered filtering styles out of snapshot output as well, but I think that's too far. That output could be useful, at least sometimes. I also thought about making this behavior configurable, but I think it unnecessarily complicates the API. I also don't see how style props are usually useful for debugging a query, and they make the output hard to grep.

Teachability, Documentation, Adoption, Migration Strategy:

I don't think this has much impact on any of these areas 🤔

@bcarroll22 bcarroll22 added enhancement New feature or request help wanted Extra attention is needed breaking change labels May 22, 2019
@lewie9021
Copy link
Contributor

This sounds like a pretty good idea. I've been using the debug method quite often, and it's usually just to check that at element exists or not. I've found that styling can useful in some cases though, particularly when the component tree has several views for layout. They kind of provide some context.

I agree that they should definitely stay in snapshots as they play a very useful role in sanity checking the rendered output.

@bcarroll22
Copy link
Collaborator Author

Thanks for the feedback @lewie9021! I think this is a good call out.

My team really only ever sees debug when the library calls it because something wasn't found, and then what we're usually looking for at that point is what is actually on the screen instead of what we thought was there. But, good point that not everyone uses it that way. At some point, depending on the feedback we get, I may just publish a beta with styles stripped out and see what folks think about it.

Just wondering, have you checked out jest-native? Especially for asserting/checking styles, it's super useful!

@lewie9021
Copy link
Contributor

I think it's great that debug is ran automatically if there is a failure and I use it for exactly that. The cases when I use it manually is usually during the creating of tests, especially when I'm dealing with complex forms. I think publishing a beta would make the most sense.

I've not just yet, but it's certainly something I'll be looking into very soon. Out of curiosity, does the disabled matcher take into account accessabilityStates too? I think it could be useful to have a selected matcher too. This is something I've had to write myself recently.

@bcarroll22
Copy link
Collaborator Author

Yep, it does take the accessibility state into account! Another thing to keep in mind if you’re looking for something that’s selected is that you can do this:

getByText(
  /hello/i,
  { selector: ({ props }) => props.accessibilityStates.includes(‘selected’) }
);

I can take a look at a pull request for a selected matcher ☺️ I’m not sure without looking at it if we’d include it, because selected is an accessibilityState which could be covered in jest-native by:

expect(element).toHaveProp(‘accessibilityState’, [‘selected’])

but I could still take a look if you don’t think these options cover your case! 💯

@lewie9021
Copy link
Contributor

Thanks for the tip on the second parameter. I didn't realise that was possible!

I think my current knowledge of the two repos is too limited to be comfortable with submitting such a PR at this time. To avoid derailing this discussion any further, I'll potentially raise an issue in the future to debate it more directly. I appreciate the alternative approaches you have suggested :)

Please keep us posted on the beta release. I'm interested in testing the proposed debug output.

@bcarroll22
Copy link
Collaborator Author

I’ve thought about this more and researched how it works with inline styles in dom-testing-library, and decided this would be too much custom behavior. This is going down a path of implementing custom behaviors that I’d really rather this library not deal with or maintain for now.

That said, it may be very possible for someone to build a plug-in for NTL that could print the native test instances this way, maybe it would be a good ecosystem package 🤷‍♂️

@ajsmth
Copy link
Contributor

ajsmth commented Oct 29, 2019

After reading this, I doubt you'll be interested but I've submitted a PR for this exact issue #70 . I'm not familiar with how a plugin would work in this instance, I would be interested in moving it to a plugin if you're not interested in adding it to this repo -- would you be willing to point me in the right direction for this?

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

No branches or pull requests

3 participants