Skip to content
This repository has been archived by the owner on Apr 11, 2023. It is now read-only.

Change the way we test snapshots #1273

Merged
merged 2 commits into from
Jun 25, 2019
Merged

Change the way we test snapshots #1273

merged 2 commits into from
Jun 25, 2019

Conversation

cfcosta
Copy link
Contributor

@cfcosta cfcosta commented Jun 24, 2019

Uses enzyme-to-json on all places instead of only the ones @jotak changed.

@cfcosta cfcosta added the do not merge A PR is not ready to merge label Jun 24, 2019
@jotak
Copy link
Contributor

jotak commented Jun 24, 2019

@cfcosta I've already added enzyme-to-json with this PR : #1270 :-(
So I guess you can revert that part of my commit (remove the dependency)

@israel-hdez
Copy link
Contributor

What I don't like too much is that attrributes are not always expanded, like:

<TablePfProvider columns={{...}} striped={true} bordered={true} hover={true} dataTable={true} className=\\"\\" condensed={false} inlineEdit={false} components={{...}}>

note the columns={{...}} and components={{...}} attributes. I also saw the same happening for style attributes.
If enzyme-to-json solves this better, I would prefer to include the dependency.

@cfcosta
Copy link
Contributor Author

cfcosta commented Jun 24, 2019

@israel-hdez this happens because it does shallow instead of complete rendering, so it does not saves snapshots when you pass objects/functions to the components.

This is necessary to have some layering on the rendering. We can use full rendering to get those to be saved as well, but that's out of the scope.

@cfcosta
Copy link
Contributor Author

cfcosta commented Jun 24, 2019

@jotak @israel-hdez changed the PR to start using enzyme-to-json on all places, can you review it again?

@cfcosta cfcosta removed the do not merge A PR is not ready to merge label Jun 24, 2019
Copy link
Contributor

@jotak jotak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this is good to me, but out of curiosity why finally enzyme-to-json was better than just .debug() ?

@cfcosta
Copy link
Contributor Author

cfcosta commented Jun 25, 2019

@jotak I'd prefer to keep debug(), but we already have enzyme-to-json on the repo, and it works. The less we change the better.

Also, @israel-hdez found some issues where important data was being hidden. To fix those problems, we would need to use .render().debug(), but it requires redux to be "running" to do that. So I decided to go the simplest way and then we can try something more complex later.

@cfcosta cfcosta merged commit b07a4ca into kiali:master Jun 25, 2019
@ghost ghost added this to the v1.1.0 milestone Jun 25, 2019
@cfcosta cfcosta deleted the jest-fixes branch June 25, 2019 14:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants