Skip to content

fix: Support diffing single-line strings #27

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

fix: Support diffing single-line strings #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
13001eb
Simplify test Component
thymikee Jan 30, 2018
cb35442
fix: Support diffing single-line strings
thymikee Jan 30, 2018
0a3fda0
address feedback
thymikee Jan 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions __tests__/__snapshots__/snapshotDiff.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,3 +134,33 @@ exports[`failed optional deps throws with sensible message on missing react-test
"Failed to load optional module \\"react-test-renderer\\". If you need to compare React elements, please add \\"react-test-renderer\\" to your project's dependencies.
Cannot find module 'non-existent-module-for-testing' from 'snapshotDiff.test.js'"
`;

exports[`supports diffing single-line strings 1`] = `
"Snapshot Diff:
- First value
+ Second value

- foo
+ bar
"
`;

exports[`supports diffing single-line strings 2`] = `
"Snapshot Diff:
- First value
+ Second value

- foo
+ bar
"
`;

exports[`supports diffing single-line strings 3`] = `
"Snapshot Diff:
- First value
+ Second value

- foo
+ bar
"
`;
76 changes: 12 additions & 64 deletions __tests__/snapshotDiff.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,83 +37,31 @@ type Props = {

class Component extends React.Component<Props> {
render() {
const dummySpans = Array(20)
.fill(0)
.map((_, index) => <span key={String(index)} />);

return (
<div>
<span />
<span />
<span />
<span>{this.props.test}</span>
<span>{this.props.test}</span>
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
{dummySpans}
<span>{this.props.test}</span>
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
<span />
{dummySpans}
</div>
);
}
}

test('supports diffing single-line strings', () => {
expect(snapshotDiff('foo', 'bar')).toMatchSnapshot();
expect(snapshotDiff('foo\n', 'bar')).toMatchSnapshot();
expect(snapshotDiff('foo', 'bar\n')).toMatchSnapshot();
});

test('collapses diffs and strips ansi by default', () => {
expect(snapshotDiff(a, b)).toMatchSnapshot();
});
Expand Down
9 changes: 9 additions & 0 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const prettyFormat = require('pretty-format');

const { ReactElement } = prettyFormat.plugins;
const reactElement = Symbol.for('react.element');
const MULTILINE_REGEXP = /[\r\n]/;

type Options = {
expand?: boolean,
Expand Down Expand Up @@ -50,6 +51,14 @@ const isReactComponent = (value: any) =>
value && value.$$typeof === reactElement;

function diffStrings(valueA: any, valueB: any, options: Options) {
// jest-diff doesn't support single-line values, but we want to
Copy link
Member

Choose a reason for hiding this comment

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

this seems weird, IMO. Maybe not, though :D

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe that's a regression?
jestjs/jest#1633
cc @pedrottimark

Copy link
Member Author

Choose a reason for hiding this comment

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

Hah, there's even a test for that.

Is this because jest-diff was supposed to provide an inline diff, but it was not supported at the time (and nothing change in that matter for now)?

Copy link

@pedrottimark pedrottimark Jan 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Y’all ask a good question about this special case. I added test to document it, not endorse it :)

I guess it’s because jest-diff omits output that seems redundant when expect calls it:

2018-01-31 single-line

but it’s confusing when snapshot-diff calls it.

Not to sound critical of awesome effort to integrate expect matchers with diff display, and then snapshots, it feels like the whole is less than the sum of the two parts in some cases. What do you think if expect omits Expected value to equal and Received lines when a diff follows? EDIT: Then jest-diff could drop the special case, as a breaking change.

Yesterday I updated a local copy of code from dormant Jest PR for inline substring diff to call diff-sequences package. This version is somewhat easier to grok than the original :)

Any feedback how to expose that new function for Jest and other callers is very welcome.

Copy link
Member Author

@thymikee thymikee Jan 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@pedrottimark you could expose some useful utils like diff lib (talking about something like diffLines and structuredPatch we use).

Back to the topic, diff + expect behavior feels like a hack to me. It's .toEquals() matcher that should be responsible for determining whether to display the diff or not.
Do you think it makes sense to file a PR to Jest?

Copy link

Choose a reason for hiding this comment

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

Yeah, we might refactor to more clearly separate assertion logic at one extreme and diff display at another extreme from the responsibility you mention, which I think is divided between expect/src/matchers.js and jest-diff/src/index.js files.

if (typeof valueA === 'string' && !MULTILINE_REGEXP.test(valueA)) {
valueA += '\n'; // eslint-disable-line no-param-reassign
}
if (typeof valueB === 'string' && !MULTILINE_REGEXP.test(valueB)) {
valueB += '\n'; // eslint-disable-line no-param-reassign
}

return diff(valueA, valueB, {
expand: options.expand,
contextLines: options.contextLines,
Expand Down