fix(testing/snapshot): handle multiline strings #2166
Conversation
| @@ -29,7 +29,7 @@ function serialize(actual: unknown): string { | |||
| compact: false, | |||
| iterableLimit: Infinity, | |||
| strAbbreviateSize: Infinity, | |||
| }); | |||
| }).replace(/\\n/g, "\n"); | |||
There was a problem hiding this comment.
With this change, the expression "..." isn't canonical string literal anymore if it contains \n.
I think it's ok and works in most cases.
But does anyone have any concern about this transformation?
There was a problem hiding this comment.
Yeah this is also how it is in Jest but that doesn't necessarily mean it's how it should be done here. The alternative I considered was:
await assertSnapshot("Hello\nWorld!");
export[`snapshot`] = `
\`Hello
World!\`
`;But I felt that this has two downsides:
- The snapshot is less readable due to the presence of escape characters
- It would require significant changes to
Deno.inspectand I was concerned about cluttering the API with options that have no use case (at least not that I can think of!) outside of snapshot testing
There was a problem hiding this comment.
I am currently working on improving the API for assertSnapshot to allow an options object. We could offer an option preserveCanonicalStringLiterals if this is something people care about.
There was a problem hiding this comment.
Thanks for the clarification.
The snapshot is less readable due to the presence of escape characters
I think readability of snapshot is more important. I'm in favor of the current approach.
I am currently working on improving the API for assertSnapshot to allow an options object. We could offer an option preserveCanonicalStringLiterals if this is something people care about.
Right, we can improve it later based on the feedbacks
There was a problem hiding this comment.
With this change, the expression
"..."isn't canonical string literal anymore if it contains\n.I think it's ok and works in most cases.
But does anyone have any concern about this transformation?
@kt3k Are we still waiting for feedback on this?
There was a problem hiding this comment.
Let's land and try to see the community feedback. Thanks for your contribution!
Fix for #2142