Add alternative serialize API for pretty-format plugins #4114

pedrottimark · 2017-07-24T17:51:52Z

Summary

Fixes #3518 with breaking changes to edge cases:

Unlike core print functions for data structures, plugins have been indenting additional lines of multiline strings. It causes additional unnecessary diff line for props if a React element moves to a different level in the render tree.
In arrays of elements (from enzyme selector, or rendered fragment in React 16)
- closing tags haven’t been indented at all (therefore, not to same level as opening tags)
- objects or elements as props values have been indented too much

New alternative API added to pretty-format and implemented in this PR for 2 plugins:

ReactElement
ReactTestComponent

export type Test = any => boolean;

export NewPrinter = (
  val: any,
  indentation: string,
  depth: number,
  refs: Refs,
) => string;

export type NewPlugin = {|
  serialize: (
    val: any,
    config: Config,
    printer: NewPrinter,
    indentation: string,
    depth: number,
    refs: Refs,
  ) => string,
  test: Test,
|};

Next step: factor out common code in these 2 plugins

Test plan

New TDD tests that failed without these improvements:

supports a single element with [EDIT: empty string child and] zero number child (unexpected discovery while proofreading :)
supports props with multiline strings
supports array of elements

The other new tests explicitly specify existing correct behavior.

Updated snapshots because these improvements omit a redundant color reset:

ReactElement plugin highlights syntax
ReactTestComponent plugin

codecov-io · 2017-07-24T18:23:09Z

Codecov Report

Merging #4114 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4114      +/-   ##
==========================================
- Coverage   60.32%   60.31%   -0.01%     
==========================================
  Files         195      195              
  Lines        6757     6756       -1     
  Branches        6        6              
==========================================
- Hits         4076     4075       -1     
  Misses       2678     2678              
  Partials        3        3

Impacted Files	Coverage Δ
.../pretty-format/src/plugins/react_test_component.js	`100% <100%> (ø)`	⬆️
packages/pretty-format/src/index.js	`98.48% <100%> (ø)`	⬆️
...ackages/pretty-format/src/plugins/react_element.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 13e375d...9550c22. Read the comment docs.

pedrottimark · 2017-07-24T22:20:28Z

Interpretation of performance change:

complex data structures are a control which suggest a measurement error about 3%
proposed API (which allows same indentation concatenation as core functions) takes about 75% of the time compared to baseline API (which calls indent function at each level of render tree)

scenario	plugins	proposed	baseline	ratio
complex	0	229280	235289	0.974
complex	9	586444	602192	0.974
react_test_component	9	705062	980415	0.719
react_element	9	829921	1133482	0.732

pedrottimark · 2017-07-25T21:03:23Z

Added a test to specify another falsey child for which the result of ReactElement plugin was inconsistent with ReactTestComponent plugin. Because assertPrintedJSX expects consistent results, formatting an element with one falsey child as empty was probably a bug not a feature.

thymikee · 2017-07-25T21:08:17Z

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

+  expect(prettyFormatElementPlugin(val, options)).toEqual(formatted);
+  expect(
+    prettyFormatBothPlugins(renderer.create(val).toJSON(), options),
+  ).toEqual(formatted);


How about:

const formattedElementPlugin = prettyFormatElementPlugin(val, options); const formattedBothPlugins = prettyFormatBothPlugins(renderer.create(val).toJSON(), options), expect(formattedElementPlugin).toEqual(formattedBothPlugins);

This way we only have one expect and still test the same thing.

One expect requires both plugins to return consistent results, which is necessary but not sufficient. I’ll replace formatted with expected to make it clearer why 2 expect are needed:

Both plugins could return consistent but incorrect results.

If one plugin is incorrect (for example, ReactElement for one falsey child) then a combined expect has a 50% chance of implying the wrong result is expected.

Something less than ideal about assertPrintedJSX is if a test fails, the message can’t identify which plugin :(

The test of array of elements caused me to factor out helper functions which I renamed and used in several test cases near the end:

formatElement

formatTestObject

thymikee · 2017-07-25T21:08:28Z

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

@@ -49,13 +53,20 @@ test('supports a single element with no props', () => {
  );
 });

-test('supports a single element with number children', () => {
+test('supports a single element with nonzero number child', () => {


nit: non-zero

Yes, thank you. And I moved the new test for empty string to make the pairs match up.

thymikee · 2017-07-25T21:16:23Z

packages/pretty-format/src/index.js

-      indentationNext + str.replace(NEWLINE_REGEXP, '\n' + indentationNext)
-    );
-  }
-


Any particular reason why these functions are now inlined?

Yes, thank you for asking. To avoid the problem in #3962 where printPlugin made unused closures for the callback functions, especially now that it might call either serialize or print property.

pedrottimark · 2017-07-26T19:28:34Z

Oh, here is a question about react.test.js which also applies to other .test.js files

const prettyFormat = require('../');
const ReactTestComponent = require('../plugins/react_test_component');
const ReactElement = require('../plugins/react_element');

Should I change it to

const prettyFormat = require('../');
const {
  ReactElement,
  ReactTestComponent,
} = prettyFormat.plugins;

to follow the pattern in

pedrottimark · 2017-07-26T22:49:30Z

Forgot to say after the first round of pull requests to replace print with serialize in plugins, the Immutable plugins can use depth and config.maxDepth just like core functions for JavaScript data structures. Probably worthwhile for HTMLElement, ReactElement, and ReactTestComponent too.

thymikee · 2017-07-27T07:26:59Z

Feel free to adjust the plugin requires to be consistent, sure!

cpojer · 2017-07-30T09:11:13Z

We are now using Flow in the test files, please rebase.

pedrottimark · 2017-07-31T17:51:35Z

Yes, Flow type for tests is a super improvement.

cpojer · 2017-08-01T08:34:16Z

Good with me. Thanks again for the hard work @pedrottimark!

* Add alternative serialize API for pretty-format plugins * Fix logic in ReactElement plugin and add test for zero number child * Add test for empty string child * Rename 2 helper functions and use in more places * Replace require paths to plugins with destructuring assignment * Add any type annotation to val arg of printPlugin * Replace overlooked occurrences of formatted with expected * Fix pretty lint error * Add comment and repeat to indent test * Add comment and repeat to indent test for react also * Fix pretty lint error in html_element.test.js

github-actions · 2021-05-11T20:06:43Z

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.

pedrottimark added 2 commits July 24, 2017 13:18

Add alternative serialize API for pretty-format plugins

58db640

Fix logic in ReactElement plugin and add test for zero number child

a5f383c

facebook-github-bot added the CLA Signed ✔️ label Jul 24, 2017

Add test for empty string child

2ff0244

thymikee reviewed Jul 25, 2017

View reviewed changes

Rename 2 helper functions and use in more places

05fc6f4

pedrottimark added 6 commits July 27, 2017 09:37

Replace require paths to plugins with destructuring assignment

ec3147e

Add any type annotation to val arg of printPlugin

ed873d6

Replace overlooked occurrences of formatted with expected

088ab74

Fix pretty lint error

bb00830

Add comment and repeat to indent test

76ec48c

Add comment and repeat to indent test for react also

9550c22

pedrottimark added 2 commits July 31, 2017 11:32

Resolve merge conflicts with Flow for tests in 4158

116fe0c

Fix pretty lint error in html_element.test.js

90ea9cd

cpojer merged commit 9665de2 into jestjs:master Aug 1, 2017

pedrottimark mentioned this pull request Aug 3, 2017

Replace print with serialize in Immutable plugins #4189

Merged

pedrottimark deleted the pretty-format-serialize branch April 6, 2019 18:32

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add alternative serialize API for pretty-format plugins #4114

Add alternative serialize API for pretty-format plugins #4114

pedrottimark commented Jul 24, 2017 •

edited

Loading

codecov-io commented Jul 24, 2017 •

edited

Loading

pedrottimark commented Jul 24, 2017

pedrottimark commented Jul 25, 2017

thymikee Jul 25, 2017

pedrottimark Jul 26, 2017

thymikee Jul 25, 2017

pedrottimark Jul 26, 2017

thymikee Jul 25, 2017

pedrottimark Jul 26, 2017

pedrottimark commented Jul 26, 2017

pedrottimark commented Jul 26, 2017

thymikee commented Jul 27, 2017

cpojer commented Jul 30, 2017

pedrottimark commented Jul 31, 2017

cpojer commented Aug 1, 2017

github-actions bot commented May 11, 2021

Add alternative serialize API for pretty-format plugins #4114

Add alternative serialize API for pretty-format plugins #4114

Conversation

pedrottimark commented Jul 24, 2017 • edited Loading

codecov-io commented Jul 24, 2017 • edited Loading

Codecov Report

pedrottimark commented Jul 24, 2017

pedrottimark commented Jul 25, 2017

thymikee Jul 25, 2017

Choose a reason for hiding this comment

pedrottimark Jul 26, 2017

Choose a reason for hiding this comment

thymikee Jul 25, 2017

Choose a reason for hiding this comment

pedrottimark Jul 26, 2017

Choose a reason for hiding this comment

thymikee Jul 25, 2017

Choose a reason for hiding this comment

pedrottimark Jul 26, 2017

Choose a reason for hiding this comment

pedrottimark commented Jul 26, 2017

pedrottimark commented Jul 26, 2017

thymikee commented Jul 27, 2017

cpojer commented Jul 30, 2017

pedrottimark commented Jul 31, 2017

cpojer commented Aug 1, 2017

github-actions bot commented May 11, 2021

pedrottimark commented Jul 24, 2017 •

edited

Loading

codecov-io commented Jul 24, 2017 •

edited

Loading