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

feat: formatting options for render() to remove options from debug() #70

Merged
merged 4 commits into from
Nov 4, 2019

Conversation

ajsmth
Copy link
Contributor

@ajsmth ajsmth commented Oct 29, 2019

What:

Added a formatting option to render() to optionally remove noisy props from debug output

Why:

A lot of times I'm not concerned with style props and they can make the debug output hard to read.

How:

Added a config parameter to the render() call that will configure prettyFormat

Checklist:

  • Documentation added to the
    docs site
  • Typescript definitions updated
  • Tests
  • Ready to be merged

I saw an earlier issue regarding something similar, and I've written an override in my custom render for projects I work on, but I thought others might want this feature as well.

@bcarroll22
Copy link
Collaborator

Nice, thanks for starting work on this! I really like the idea and think it will be useful.

I'll have a couple of requests to discuss before we merge. Also note that we'll want to document it and make sure the build is passing (right now it's failing on coverage). I don't have enough time to fully respond at the moment but I'll try to circle back to this tonight.

Thanks again for getting this going! Looking forward to merging this feature 👍

@ajsmth
Copy link
Contributor Author

ajsmth commented Oct 29, 2019

Okay awesome — totally forgot to test coverage 😬I can fix that up. Also wasn’t sure how to update the docs, are there docs on updating the docs? lol cheers

@codecov
Copy link

codecov bot commented Oct 29, 2019

Codecov Report

Merging #70 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #70   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          20     20           
  Lines         266    268    +2     
  Branches       67     67           
=====================================
+ Hits          266    268    +2
Impacted Files Coverage Δ
src/lib/pretty-print.js 100% <100%> (ø) ⬆️
src/lib/to-json.js 100% <100%> (ø) ⬆️

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 94406cd...53cf44e. Read the comment docs.

@lewie9021
Copy link
Contributor

This is a really neat feature!

I too would like to omit particular props from the output. For me it's the style prop and the data prop which I often find pushed on to the ScrollView from a FlatList (though probably a bug).

Would it make better sense to have this take a function for formatting (a little like JSON.stringify)? That way we could provide a lot more flexibility via composition. If not, I would say omitProps would be a more fitting property name, personally.

One little note on the tests. You want to avoid "global" variables (such as log) as they often lead to bugs due to state bleeding over into other tests. At the very least, it should be reset between tests.

@ajsmth
Copy link
Contributor Author

ajsmth commented Oct 30, 2019

cool -- sounds good. can you provide a little snippet for what you mean for the function? I think I initially wanted to provide something similar, but I'm not sure we're on the same page

i'll reset log afterEach(), but i'd be curious to know if there's another approach as to getting the output!

@bcarroll22
Copy link
Collaborator

Alright sorry for the delay here.

I did some testing a while ago, and it turns out the options parameter on the render method that we pass through to react-test-renderer doesn't actually do anything. The only option is createHostNode and for some reason in React Native, it doesn't pay attention to that option. I would like to sunset that pass-through, and re-purpose the options array for things like this instead. So rather than the new API being render(Component, { formatting: {} }), it would instead be render(Component, { options: { formatting: {} } }).

The reason for this change is staying as close as possible to the top-level API of the rest of the Testing Library family. So I think what I personally would like to see the final API for be would be:

render(Component, { options: { debug: { omitProps: [] } } })

I like the name suggestion @lewie9021 made, and if you two want to further discuss the option of allowing a function as the prop, I'm cool with that as well. I'd also like to get @TAGraves opinion and /cc @alexkrolick since this is a minor deviation from the top-level API of testing library on both render and debug.

Once we have a final draft of the PR, I'll do a more thorough code review before merging. Thanks again! 👍

@bcarroll22
Copy link
Collaborator

One last thought that came to me as soon as I posted the comment is that there's already some stuff in place that does something like this. Currently we filter out any props in debug output where the value of the prop is a function. It just occurred to me that this should either take the place of that or build on-top of it.

That happens here. Ultimately the result of toJSON is what gets passed to debug. This would probably be a good place, because it would also keep us in sync with the API of Testing Library 😄

@ajsmth
Copy link
Contributor Author

ajsmth commented Oct 30, 2019

amazing, thanks for finding that. it will be a lot simpler to implement there than to tack it on to prettyPrint. i totally agree about keeping the api as similar as possible.

@ajsmth
Copy link
Contributor Author

ajsmth commented Oct 30, 2019

updated the PR -- one last thought I had was that prettyPrint could be passed the debug options as a second argument instead of an optional maxLength, that way it could be further customized in individual render() calls (e.g maxLength, or any other future configuration)

@bcarroll22
Copy link
Collaborator

Alright I'm good to move forward with this. If there's anything else that comes up, we can iterate and merge later 👍

@bcarroll22 bcarroll22 merged commit a8ba416 into testing-library:master Nov 4, 2019
@bcarroll22
Copy link
Collaborator

🎉 This PR is included in version 4.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants