add Immutable pretty-format plugins for Immutable.OrderedSet and Immu… #2899

cpenarrieta · 2017-02-16T02:42:29Z

Comparing Immutable objects should show a more descriptive failure message

Summary
fixes #2489
I am checking if an object is Immutable.List the same way immutable source code does: https://github.com/facebook/immutable-js/blob/master/src/List.js#L201
I am checking if an object is Immutable.OrderedSet the same way immutable source code does: https://github.com/facebook/immutable-js/blob/master/src/OrderedSet.js#L44

Test plan
Testing it from an external project

I also created unit test to test the new plugins

…table.List

cpenarrieta · 2017-02-16T02:47:35Z

I can add the rest of Immutable data structures once we agree this is the way to go. Thanks for reviewing this PR.

thymikee · 2017-02-16T07:09:02Z

Hm, I think it would be worth to add all Immutable collections instead of just 2.

thymikee · 2017-02-16T07:10:50Z

Also, how about calling it ImmutableJSPlugin and store it in one file?

cpenarrieta · 2017-02-16T07:17:48Z

Ok I'll work on the rest of immutable data structures.
I thought you wanted one plugin per immutable data structures (#2529 (comment))
I can put it all in one file if you want.

thymikee · 2017-02-16T07:27:24Z

Oh, I didn't remember that. Now after reading this thread again I'm not super sure using Immutable.toString() is best here, as it would cause troubles with rendering nested data structures we already pretty-print, like React components.
Maybe lets wait for @cpojer to weigh in here.

cpojer · 2017-02-16T07:58:20Z

@thymikee if I remember correctly I made that exact same comment on the previous PR. We cannot use Immutable.toString here because of the problem you mentioned.

cpenarrieta · 2017-02-16T08:50:08Z

I think the previous PR comment was about not relying on toString() when we are checking if its an immutable data structures or not.
In my case, I'm checking if its an immutable data structure in a different way.
In my case I'm using toString() in the print function only.
Let me know if thats still an issue. If thats the case, would you recommend using pretty-immutable?

cpojer · 2017-02-16T09:00:47Z

I don't think we should call toString and materialize the entire immutable object just to know which type it is.

cc @leebyron can you help us out a bit? What's the best way to find out whether a value is from immutable.js and what type it is without explicitly requiring immutable.js?

cpenarrieta · 2017-02-16T09:03:14Z

@cpojer I am not using toString to know if its immutable or not.
Please see https://github.com/facebook/jest/pull/2899/files#diff-f105679dfd19fb268caec3ea3d5df94bR25 as an example.
printImmutableList will only be called if test is true.

cpenarrieta · 2017-02-16T09:05:30Z

@cpojer I am checking if an object is Immutable.List the same way immutable source code does: https://github.com/facebook/immutable-js/blob/master/src/List.js#L201
Same for the rest of the data structures

codecov-io · 2017-02-17T22:59:14Z

Codecov Report

Merging #2899 into master will increase coverage by 0.45%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2899      +/-   ##
==========================================
+ Coverage   67.86%   68.32%   +0.45%     
==========================================
  Files         147      155       +8     
  Lines        5349     5436      +87     
==========================================
+ Hits         3630     3714      +84     
- Misses       1719     1722       +3

Impacted Files	Coverage Δ
...ackages/pretty-format/src/plugins/ImmutableList.js	`100% <100%> (ø)`
packages/jest-diff/src/index.js	`79.59% <100%> (-3.75%)`	❌
packages/pretty-format/src/plugins/ImmutableSet.js	`100% <100%> (ø)`
...ckages/pretty-format/src/plugins/ImmutableStack.js	`100% <100%> (ø)`
packages/jest-matcher-utils/src/index.js	`72.22% <100%> (+0.79%)`	✅
...s/pretty-format/src/plugins/ImmutableOrderedMap.js	`100% <100%> (ø)`
...ages/pretty-format/src/plugins/ImmutablePlugins.js	`100% <100%> (ø)`
...s/pretty-format/src/plugins/ImmutableOrderedSet.js	`100% <100%> (ø)`
...ckages/pretty-format/src/plugins/printImmutable.js	`100% <100%> (ø)`
packages/pretty-format/src/plugins/ImmutableMap.js	`100% <100%> (ø)`
... and 11 more

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 9ff938a...7d6143d. Read the comment docs.

cpenarrieta · 2017-02-23T21:37:49Z

@cpojer @leebyron can you take a look at this again.
The way I'm checking if an object is immutable or not is by looking at their sentinel key like immutable project does. See a couple of examples https://github.com/facebook/immutable-js/blob/master/src/List.js#L201 and https://github.com/facebook/immutable-js/blob/master/src/OrderedSet.js#L44
If the object is immutable then we will call the toString() method. We won't call that method if the toString() object is not immutable.
Thanks for reviewing

thymikee · 2017-02-23T22:11:08Z

@cpenarrieta the way you test for Immutable is totally OK.

It's just Immutable#toString is not as powerful as we'd like it to be, but maybe it's good for starters? BTW, can you test how it renders a set of react components?

It's also great that you use Immutable as a devDependency, because it's only needed for tests (although most of them could be replaced with snapshots IMO).

cpenarrieta · 2017-02-23T23:29:39Z

@thymikee an Immutable.Set with react components doesn't look good
I'm working on the fix
Does this looks good?

cpenarrieta · 2017-02-24T02:43:40Z

@thymikee support for React components inside Immutable elements is done

cpenarrieta · 2017-02-27T22:42:48Z

@thymikee can you look at this again? I did some refactor of the code.
I'm now not using toString
Thanks

thymikee

This is great so far. I've left some inline comments and thoughts

thymikee · 2017-02-28T10:27:15Z

packages/pretty-format/src/plugins/printImmutable.js

+    if (reactPlugin.test(item)) {
+      result += addKey(isMap, key) + 
+        reactPlugin.print(item, print, indent, opts, colors) + ', ';
+    } else {


We need to remove this, as we don't want Immutable plugin to be dependent on ReactElement plugin. Instead they need to cooperate nicely (which is actually done already!)

You can change this to:

val.forEach((item: any, key: any) => { result += addKey(isMap, key) + print(item, print, indent, opts, colors) + ', '; });

thymikee · 2017-02-28T10:32:32Z

packages/pretty-format/src/plugins/printImmutable.js

+    return result + ' []';
+  }
+
+  result += ' [ ';


There's a opts argument in print() which looks like this:

{ edgeSpacing: '\n', min: false, spacing: '\n' } // when {min: false} { edgeSpacing: '', min: true, spacing: ' ' } // when {min: true}

Can you make sure it renders nicely for both cases?

thymikee · 2017-02-28T10:34:00Z

packages/pretty-format/src/__tests__/ImmutablePlugins-test.js

+    const reactComponent = React.createElement('Mouse', null, 'Hello World');
+    assertImmutableObject(
+      Immutable.OrderedSet([reactComponent, reactComponent]),
+      'Immutable.OrderedSet [ <Mouse>\n  Hello World\n</Mouse> ]'


In this case you should include ReactElement and ReactTestComponent in plugins

thymikee · 2017-02-28T10:37:17Z

packages/pretty-format/src/__tests__/ImmutablePlugins-test.js

+  it('supports multiple string elements', () => {
+    assertImmutableObject(
+      Immutable.OrderedSet(['jhon', 'mike', 'cristian']),
+      'Immutable.OrderedSet [ "jhon", "mike", "cristian" ]'


For {min: true} I'd rather print:

Immutable.OrderedSet ["jhon", "mike", "cristian"]

and for {min: false}:

Immutable.OrderedSet [ "jhon", "mike", "cristian", ]

Just like it works for arrays and objects now.

The same applies to all other Immutable entities

thymikee · 2017-02-28T10:41:54Z

packages/jest-matcher-utils/src/index.js

+  ImmutableMapPlugin,
+  ImmutableSetPlugin,
+  ImmutableStackPlugin,
+];


We also use plugins in jest-diff and jest-snapshot.
Alos, maybe we could create ImmutablePlugins.js which exposes an array of all Immutable plugins?

cpenarrieta · 2017-03-01T05:19:35Z

@thymikee I think I solved all your observations, please take a look again.

thymikee

Last 2 changes and I think we're good to go.
cc: @cpojer

thymikee · 2017-03-01T07:14:33Z

packages/pretty-format/src/__tests__/ImmutablePlugins-test.js

+    assertImmutableObject(
+      Immutable.OrderedSet(['jhon', 'mike', 'cristian']),
+      'Immutable.OrderedSet [\n  "jhon",\n  "mike",\n  "cristian",\n]',
+      {min: false}


min is set to false by default, so it's not necessary here

thymikee · 2017-03-01T07:36:32Z

packages/jest-diff/src/index.js

@@ -30,6 +31,7 @@ const PLUGINS = [
  ReactTestComponentPlugin,
  ReactElementPlugin,
  AsymmetricMatcherPlugin,
+  ...ImmutablePlugins,


This will fail on Node 4, as array spread is supported only behind a flag. You can use concat.

Oh, looks like it works

you are right, it fails in circleCI. its fixed now.

cpojer · 2017-03-03T13:53:09Z

package.json

@@ -30,7 +30,8 @@
    "react-test-renderer": "^15.4.1",
    "rimraf": "^2.5.4",
    "strip-ansi": "^3.0.1",
-    "typescript": "^2.1.4"
+    "typescript": "^2.1.4",
+    "immutable": "^3.8.1"


can you insert this in the right alphabetical location please?

cpojer · 2017-03-03T13:54:30Z

packages/pretty-format/src/plugins/printImmutable.js

+    return result.slice(0, -2) + opts.edgeSpacing + closeTag;
+  } else {
+    //remove last spacing
+    return result.slice(0, -1) + opts.edgeSpacing + closeTag;


Instead of using slice here, can we build up an array and call join(',' + opts.spacing) down here?

cpojer · 2017-03-03T13:55:25Z

packages/pretty-format/src/plugins/ImmutableStack.js

+
+module.exports = {
+  print: printImmutableStack,
+  test: (object: Object) => object && isStack(object),


Could we call each function in every plugin simply "print" and "test"?

That way we can do:

module.exports = {print, test};

and everything will look consistent across plugins.

cpojer · 2017-03-03T13:56:04Z

Nice! cc @leebyron do you have any comments about this? :)

cpojer · 2017-03-03T13:57:31Z

packages/pretty-format/src/__tests__/ImmutablePlugins-test.js

+const ImmutablePlugins = require('../plugins/ImmutablePlugins');
+
+function assertImmutableObject(actual, expected, opts) {
+  expect(


Could we use expect.extend instead?

See http://facebook.github.io/jest/docs/expect.html#expectextendmatchers

expect(immutableValue).toPrettyPrintTo('Immutable.OrderedSet')

or something like this?

cpenarrieta · 2017-03-03T18:58:37Z

@cpojer I solved all your observations but the use of expect.extend.
I'll try to solve it later tonight.

cpenarrieta · 2017-03-04T02:07:14Z

@thymikee @cpojer I refactored the tests to use expect.extend
Let me know I need any other refactor, thanks.

thymikee · 2017-03-04T07:41:36Z

Looks good to me

cpojer · 2017-03-06T13:34:05Z

packages/pretty-format/src/__tests__/ImmutablePlugins-test.js

+    );
+    const pass = prettyPrintImmutable === expected;
+
+    const message = pass


wow, lovely! Thank you so much @cpenarrieta

cpojer · 2017-03-06T13:35:46Z

packages/pretty-format/src/plugins/ImmutableList.js

+  print: Function,
+  indent: Function,
+  opts: Object,
+  colors: Object


the flow typing here could be a lot stronger. @cpenarrieta we just added flow types to pretty-format itself – mind sending a second, follow-up PR, where you use the types from the main module? You can extract it into a types.js file like we do in other packages.

I'll work on it
thanks

cpojer · 2017-03-06T13:37:23Z

Solid work, thanks @cpenarrieta!

jestjs#2899) * add Immutable pretty-format plugins for Immutable.OrderedSet and Immutable.List * adding immutable pretty-format plugins * update yarn.lock with immutable library * adding support for react components in Immutable pretty-format plugins * adding copyright header * refactor printImmutable method * fix flow check * add support for opts {min: false} for immutable plugins * change ES2015 spread syntax to .concat * add missing semicolon * refactor printImmutable * adding trailing comma when {min: false} * refactor immutable tests using expect.extend toPrettyPrintTo

github-actions · 2021-05-13T21:06:52Z

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.

add Immutable pretty-format plugins for Immutable.OrderedSet and Immu…

899ef77

…table.List

facebook-github-bot added the CLA Signed ✔️ label Feb 16, 2017

adding immutable pretty-format plugins

0ad8b38

cpenarrieta added 2 commits February 16, 2017 09:30

update yarn.lock with immutable library

4c75854

Merge branch 'master' into issue-2489-pretty-immutable

2cd9b50

cpenarrieta added 2 commits February 23, 2017 18:03

adding support for react components in Immutable pretty-format plugins

e981653

Merge branch 'master' into issue-2489-pretty-immutable

457d6b6

cpenarrieta added 4 commits February 23, 2017 20:26

adding copyright header

1e49f13

refactor printImmutable method

e7727d2

fix flow check

abf1523

Merge branch 'master' into issue-2489-pretty-immutable

bf7cf8a

thymikee requested changes Feb 28, 2017

View reviewed changes

add support for opts {min: false} for immutable plugins

c12b8ac

thymikee approved these changes Mar 1, 2017

View reviewed changes

cpenarrieta added 2 commits February 28, 2017 23:56

change ES2015 spread syntax to .concat

a4bdc74

add missing semicolon

1402e2d

cpojer reviewed Mar 3, 2017

View reviewed changes

refactor printImmutable

81aebeb

cpenarrieta added 2 commits March 3, 2017 15:45

adding trailing comma when {min: false}

0f58eb2

refactor immutable tests using expect.extend toPrettyPrintTo

7d6143d

cpojer reviewed Mar 6, 2017

View reviewed changes

cpojer merged commit 25e9145 into jestjs:master Mar 6, 2017

renchap mentioned this pull request May 30, 2017

Add support for Immutable.Record in pretty-format #3678

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

add Immutable pretty-format plugins for Immutable.OrderedSet and Immu… #2899

add Immutable pretty-format plugins for Immutable.OrderedSet and Immu… #2899

Conversation

cpenarrieta commented Feb 16, 2017 • edited Loading

cpenarrieta commented Feb 16, 2017

thymikee commented Feb 16, 2017

thymikee commented Feb 16, 2017

cpenarrieta commented Feb 16, 2017

thymikee commented Feb 16, 2017

cpojer commented Feb 16, 2017

cpenarrieta commented Feb 16, 2017 • edited Loading

cpojer commented Feb 16, 2017

cpenarrieta commented Feb 16, 2017 • edited Loading

cpenarrieta commented Feb 16, 2017 • edited Loading

codecov-io commented Feb 17, 2017 • edited Loading

Codecov Report

cpenarrieta commented Feb 23, 2017

thymikee commented Feb 23, 2017

cpenarrieta commented Feb 23, 2017

cpenarrieta commented Feb 24, 2017

cpenarrieta commented Feb 27, 2017

thymikee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpenarrieta commented Mar 1, 2017

thymikee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Mar 3, 2017

Choose a reason for hiding this comment

cpenarrieta commented Mar 3, 2017

cpenarrieta commented Mar 4, 2017

thymikee commented Mar 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer commented Mar 6, 2017

github-actions bot commented May 13, 2021

cpenarrieta commented Feb 16, 2017 •

edited

Loading

cpenarrieta commented Feb 16, 2017 •

edited

Loading

cpenarrieta commented Feb 16, 2017 •

edited

Loading

cpenarrieta commented Feb 16, 2017 •

edited

Loading

codecov-io commented Feb 17, 2017 •

edited

Loading