Allow multiline snapshot label/message

[KillyMXI]

Relevant code:

ava/lib/snapshot-manager.js

Line 96 in a8c908e

return `> ${label}\n\n${indentString(description, 4)}`;

I'd expect something like label.split(/\n/).map(line => '> ' + line).join('\n').
Or an option "I would like to format it myself" (Don't insert blockquote or anything - I'll pass the formatted text instead.)

Why:
I'd love to have snapshot reports as human readable and illustrative as possible to serve as a user documentation supplement.
Currently I have to produce not quite readable one-liners, as can be seen here.

I plan to migrate a bigger project to AVA, and it should greatly benefit from the snapshots feature. But I also expect I might need to put more content into labels there.

[novemberborn]

Official multiline support makes a lot of sense. Would you like to work on it?

Incidentally I think we may have a similar problem with test titles.

[KillyMXI]

Hmm. Test titles? Do you mean you put code examples into test titles? And how that supposed to be rendered in markdown? Currently it's just ## headers.

I think I can work on this as long as we agree on the design and it wouldn't grow much in complexity. One design question I've outlined already - it's whether snapshot label/message should be put inside a blockquote or whether there can/should be other way to handle it.

[novemberborn]

Sometimes test titles have new lines which break formatting in the snapshot reports. Those should probably be stripped so the title is on a single line, unless Markdown let's you hard-wrap headings.

For individual snapshots we should render multi line block quotes.

[KillyMXI]

Ah ok. Sanitizing titles makes sense. Although I wouldn't quite call it similar with this.

Gotcha on multi line block quotes.

[novemberborn]

You're right it's different, your suggestion just reminded me of it. Happy to tackle as a separate piece of work 😊

[KillyMXI]

I think title.replace(/\s+/g, ' ') might be a good way to sanitize the title. Unless I'm missing something and some whitespace characters have to be preserved.

What I find the most intimidating now is to find the right place where to put tests.
It looks to me that it will require new file(s) but it's not obvious to me where to put them.
Looks like test folder is the right one, but what would be the area of responsibility? There are multiple snapshot-related subfolders. I'm not sure whether this belongs to one of them (like snapshot-tests - pretty generic one) or a new one (like snapshot-reports).

I can provide two separate commits/PRs for these. If this falls into the same new tests category then it will be easier to handle in a single PR.

[novemberborn]

snapshot-tests should be fine. The idea is to have a fixture under say test/snapshot-tests/fixtures/formatting that saves snapshots with various test titles and snapshot labels. You then run this fixture from a test/snapshot-tests/formatting.js file and use that to snapshot the snapshot report. See https://github.com/avajs/ava/blob/main/test/snapshot-order/randomness.js for an example.

It's a bit roundabout but it means we have an integration test for this behavior that we perform using a stable AVA version.

Happy to help if you get stuck.

[ninevra]

Looks like test folder is the right one, but what would be the area of responsibility? There are multiple snapshot-related subfolders. I'm not sure whether this belongs to one of them (like snapshot-tests - pretty generic one) or a new one (like snapshot-reports).

This is sort of an accidental artifact of features being added over time; the first one(s) got subfolders named after the feature, presumably not anticipating there being quite this many tests, and then later ones followed the pattern. It's also awkward that test/snapshots is for storing snapshots for tests, not storing tests for snapshots >_<.

Personally I'd advocate relocating snapshot-order to snapshot-tests/order etc. now that a generically-named folder (snapshot-tests) exists, but I imagine that refactor, if approved, would be its own PR.

[KillyMXI]

House-keeping is certainly not in scope of this issue.

[KillyMXI]

Ok, after messing around with tests for a while I think I have it working, albeit with cjs/require rather than module/import inside fixtures.

Snapshot label is a pretty straightforward fix.
But test title raises another question:
What is the right point in the test lifecycle where the title should be sanitized...
Only fixing it in the report generator will mean extra whitespace will be present in stdout and all internal processes (checking for the same entries presence in snapshots for example, if I understood that correctly).

It seems to be a good idea to have titles sanitized as early as possible but that is going to be a breaking change (albeit only in cases affected by extra spaces/line breaks). Is it OK?

It is also not so trivial for me to identify the right location in code.

ava/lib/parse-test-args.js

Line 2 in a8c908e

const rawTitle = typeof args[0] === 'string' ? args.shift() : undefined;

seems to be a good candidate.
And I'm not sure whether the line

ava/lib/test.js

Line 96 in a8c908e

title = `${test.title} ─ ${title}`;

is safe. Have to include a test for try assertions probably.

Snapshot report and stdout are pretty obvious things to test. Are there any other places to test the title?

Making the change backwards-compatible by comparing sanitized titles everywhere (I haven't looked into how many relevant places there might be) - sounds like snowballing the issue further more.

[KillyMXI]

I made a PR #2773 with snapshot labels only for now.

[novemberborn]

@KillyMXI I've opened #2774 to track the test title issue.