Skip to content
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

Add pretty-inmutable for better error message [issue-2489] #2529

Conversation

luiskhernandez
Copy link
Contributor

When comparing against inmutable value show a more
descriptive message

Summary
This pr attempt to fix #2489 by adding pretty-inmutable to

Test plan
run this test

it('compares Immutable-js OrderedSet', () => {
  expect(Immutable.OrderedSet(['foo'])).toEqual(['foo']);
});

it('compares Immutable-js List', () => {
  expect(Immutable.List(['foo'])).toEqual(['foo']);
});

and you should see.

screen shot 2017-01-07 at 3 46 11 pm

When comparing against inmutable value show a more
descriptive message
@codecov-io
Copy link

codecov-io commented Jan 7, 2017

Current coverage is 62.18% (diff: 12.50%)

Merging #2529 into master will decrease coverage by 0.08%

@@             master      #2529   diff @@
==========================================
  Files           116        117     +1   
  Lines          4802       4810     +8   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2990       2991     +1   
- Misses         1812       1819     +7   
  Partials          0          0          

Powered by Codecov. Last update 742eb0b...b47927d

/* eslint-disable max-len */
'use strict';
const prettyI = require('pretty-immutable');
const InmutableTypes = ['List', 'OrderedSet'];
Copy link
Member

Choose a reason for hiding this comment

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

There is a typo here and this list is not exhaustive either.

'use strict';
const prettyI = require('pretty-immutable');
const InmutableTypes = ['List', 'OrderedSet'];
const isInMutable = val => {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem like an efficient way to determine what type it is.

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Thank you for this PR but I think there is a number of things that need to be changed here:

  • We need to build our completely own pretty-printing for all immutable types. Otherwise we hand off the work to a different pretty-printer as soon as we see the first immutable item, which isn't really useful if we have other objects (like react elements) in an immutable list.
  • immutable should be an optional dependency and pretty-format must not depend on it.
  • Use this plugin everywhere we use prettyFormat inside of Jest.

@luiskhernandez
Copy link
Contributor Author

Hey @cpojer. Thanks for the feedback. couple of questions and i'll try again.

Do you mean, to create a print function like printArguments, printArray, printSet that are in https://github.com/facebook/jest/blob/master/packages/pretty-format/src/index.jsbut for all the Inmutable types, and add them to the a similar printComplexValue as a pretty-print plugin?

Any hint on how to detect if it is a imputable type with out using Inmutable?

I'll keep hacking to see how can in improve the pr, thanks.

@cpojer
Copy link
Member

cpojer commented Jan 9, 2017

Yeah, you should create one plugin for each type from immutable we'd like to pretty-print. Note: the library is called "immutable" not "inmutable".

I'll close this PR for now as it will likely need a couple of iterations to get ready. Feel free to push to this one and notify me by commenting here or create a new PR when you are ready :)

@cpojer cpojer closed this Jan 9, 2017
@thymikee
Copy link
Collaborator

thymikee commented Jan 9, 2017

I'm also not sure if we want add extra dependency of pretty-immutable.

@luiskhernandez
Copy link
Contributor Author

luiskhernandez commented Feb 14, 2017

@thymikee @cpojer Hey guys, i was giving this a try again, and i was wondering if we can rely on the toString method that is already built in the immutable see example

@thymikee
Copy link
Collaborator

I think this can be a good base. But we need to prefix this with "Immutable" and make sure it behaves well with min option

@luiskhernandez
Copy link
Contributor Author

@thymikee can you give a example on how should behave with min option and other expected results to used them as test scenarios?

@thymikee
Copy link
Collaborator

Min set to true will behave like immutable.toString. When set to false it will render each value in new line. You can see this behaviour when testing arrays in Jest

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error message when comparing Immutable-js collection to array is confusing
5 participants