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.

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

Jump to bottom

jest-snapshot: Display change counts in annotation lines #8982

Merged
merged 7 commits into from Oct 9, 2019
Merged

jest-snapshot: Display change counts in annotation lines #8982

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c8e7667
jest-snapshot: Display change counts in annotation lines
pedrottimark Sep 25, 2019
b39fb4d
Edit comment
pedrottimark Sep 25, 2019
3a96054
Update CHANGELOG.md
pedrottimark Sep 25, 2019
9d13183
Move indicators for change counts from postfix to prefix
pedrottimark Sep 26, 2019
db494cb
Display with labels if neither quoted string is multiline
pedrottimark Sep 27, 2019
891c953
Delete obsolete work-around variable assignment
pedrottimark Sep 30, 2019
96389e7
Rename Padding variables
pedrottimark Sep 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 10 additions & 4 deletions e2e/__tests__/__snapshots__/failures.test.ts.snap
View file
Open in desktop
Expand Up @@ -786,8 +786,11 @@ FAIL __tests__/snapshot.test.js

Snapshot name: \`failing snapshot 1\`

Snapshot: "bar"
Received: "foo"
- Snapshot 1 -
+ Received 1 +

- bar
+ foo
Copy link
Contributor

Choose a reason for hiding this comment

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

For this case, where it's a string that was 1 line and will again be 1 line, this information is pretty useless. Can we not display it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will deal with comparison of two one-line strings as a special case for the concise labeled format, because it seems likely for toThrowErrorMatchingSnapshot assertions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe with a clearer labeling (other comment thread) it's actually fine and worth keeping for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tim, good point about prioritizing consistency higher than a more concise special case.

If most reports have differences when snapshot assertions fail, and if next pull request changes the colors to be less confusing, some people might learn to scan quickly which console output is from snapshots versus other assertions, and more intuitively choose which decision path.

For example, the e2e tests for this pull request had 3 snapshots to update and 1 toMatch assertion which I needed to rewrite as 4 assertions.

To test this theory: think fast, what do you need to do for each of the following:

Expected: "bar"
Received: "foo"

Snapshot: "bar"
Received: "foo"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I know what you mean, if there's lots of (correct) test failures I usually try to change non-snapshot tests first and then update snapshots once they're the only things still failing. Not sure how that's related to this comment thread but I'd definitely appreciate making it easier to tell them apart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See pictures in #8982 (comment) of concise label format if neither string is multiline


9 |
10 | test('failing snapshot', () => {
Expand All @@ -812,8 +815,11 @@ FAIL __tests__/snapshotWithHint.test.js

Snapshot name: \`failing snapshot with hint: descriptive hint 1\`

Snapshot: "bar"
Received: "foo"
- Snapshot 1 -
+ Received 1 +

- bar
+ foo

9 |
10 | test('failing snapshot with hint', () => {
Expand Down
6 changes: 4 additions & 2 deletions e2e/__tests__/__snapshots__/watchModeUpdateSnapshot.test.ts.snap
View file
Open in desktop
Expand Up @@ -6,8 +6,10 @@ FAIL __tests__/bar.spec.js
● bar
expect(received).toMatchSnapshot()
Snapshot name: \`bar 1\`
Snapshot: "foo"
Received: "bar"
- Snapshot 1 -
+ Received 1 +
- foo
+ bar
1 |
> 2 | test('bar', () => { expect('bar').toMatchSnapshot(); });
| ^
Expand Down
6 changes: 5 additions & 1 deletion e2e/__tests__/toMatchSnapshot.test.ts
View file
Open in desktop
Expand Up @@ -108,7 +108,11 @@ test('first snapshot fails, second passes', () => {
writeFiles(TESTS_DIR, {[filename]: template([`'kiwi'`, `'banana'`])});
const {stderr, exitCode} = runJest(DIR, ['-w=1', '--ci=false', filename]);
expect(stderr).toMatch('Snapshot name: `snapshots 1`');
expect(stderr).toMatch('Snapshot: "apple"\n Received: "kiwi"');
// Match lines separately because empty line has been replaced with space:
expect(stderr).toMatch('- Snapshot 1 -');
expect(stderr).toMatch('+ Received 1 +');
expect(stderr).toMatch('- apple');
expect(stderr).toMatch('+ kiwi');
expect(stderr).not.toMatch('1 obsolete snapshot found');
expect(exitCode).toBe(1);
}
Expand Down
42 changes: 33 additions & 9 deletions packages/jest-matcher-utils/src/__tests__/printDiffOrStringify.test.ts
View file
Open in desktop
Expand Up @@ -6,7 +6,7 @@
*/

import {alignedAnsiStyleSerializer} from '@jest/test-utils';
import {EXPECTED_COLOR, INVERTED_COLOR, printDiffOrStringify} from '../index';
import {INVERTED_COLOR, printDiffOrStringify} from '../index';

expect.addSnapshotSerializer(alignedAnsiStyleSerializer);

Expand Down Expand Up @@ -50,16 +50,40 @@ describe('printDiffOrStringify', () => {
expect(testDiffOrStringify(expected, received)).toMatchSnapshot();
});

test('received is multiline longer than max', () => {
const expected = 'multi\nline';
const received = 'multi' + '\n123456789'.repeat(2000); // 5 + 20K chars
describe('MAX_DIFF_STRING_LENGTH', () => {
const lessChange = INVERTED_COLOR('single ');
const less = 'single line';
const more = 'multi line' + '\n123456789'.repeat(2000); // 10 + 20K chars

test('both are less', () => {
const difference = testDiffOrStringify('multi\nline', less);

expect(difference).toMatch('- multi');
expect(difference).toMatch('- line');

// diffStringsUnified has substring change
expect(difference).not.toMatch('+ single line');
expect(difference).toMatch(lessChange);
});

test('expected is more', () => {
const difference = testDiffOrStringify(more, less);

expect(difference).toMatch('- multi line');
expect(difference).toMatch('+ single line');

// diffLinesUnified does not have substring change
expect(difference).not.toMatch(lessChange);
});

const test = testDiffOrStringify(expected, received);
test('received is more', () => {
const difference = testDiffOrStringify(less, more);

// It is a generic line diff:
expect(test).toContain(EXPECTED_COLOR('- line'));
expect(difference).toMatch('- single line');
expect(difference).toMatch('+ multi line');

// It is not a specific substring diff
expect(test).not.toContain(EXPECTED_COLOR('- ' + INVERTED_COLOR('line')));
// diffLinesUnified does not have substring change
expect(difference).not.toMatch(lessChange);
});
});
});
11 changes: 7 additions & 4 deletions packages/jest-snapshot/src/State.ts
View file
Open in desktop
Expand Up @@ -12,10 +12,10 @@ import {getStackTraceLines, getTopFrame} from 'jest-message-util';
import {
getSnapshotData,
keyToTestName,
removeExtraLineBreaks,
saveSnapshotFile,
serialize,
testNameToKey,
unescape,
} from './utils';
import {InlineSnapshot, saveInlineSnapshots} from './inline_snapshots';
import {SnapshotData} from './types';
Expand All @@ -29,7 +29,7 @@ export type SnapshotStateOptions = {

export type SnapshotMatchOptions = {
testName: string;
received: any;
received: unknown;
key?: string;
inlineSnapshot?: string;
isInline: boolean;
Expand Down Expand Up @@ -253,9 +253,12 @@ export default class SnapshotState {
if (!pass) {
this.unmatched++;
return {
actual: unescape(receivedSerialized),
actual: removeExtraLineBreaks(receivedSerialized),
count,
expected: expected !== undefined ? unescape(expected) : undefined,
expected:
expected !== undefined
? removeExtraLineBreaks(expected)
: undefined,
key,
pass: false,
};
Expand Down