-
Notifications
You must be signed in to change notification settings - Fork 653
feat(testing): add assertInlineSnapshot()
#6530
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.
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6530 +/- ##
==========================================
- Coverage 94.75% 94.44% -0.31%
==========================================
Files 584 587 +3
Lines 46560 46825 +265
Branches 6539 6545 +6
==========================================
+ Hits 44116 44224 +108
- Misses 2401 2558 +157
Partials 43 43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Thanks for the PR, but this seems only supporting the initial creation of snapshot. In my view the capability of updating the snapshots are essential for snapshot testing tool. Can we also support the updating somehow? (I guess we can do that only by using AST analysis as jest does) |
I agree that this limitation of You can update a snapshot by manually replacing the existing one with `CREATE` and running the test. This does force users to follow the commonly recommended practice of ensuring each updated snapshot should actually have been updated. Also, users always have the option of If we wanted to support updating failing snapshots automatically, it's likely that the best solution would involve AST analysis. I don't think Deno exposes its AST parser, and Jest uses Babel which is presumably something we cannot do here. So a system using AST analysis is not likely to be ready anytime soon. I'm not sure, but I suspect that even without the ability to update automatically, users might find this feature useful. On the other hand, I do recognize the need to maintain high quality throughout the std library, and shipping an incomplete feature is not great either. |
So if I understand this correctly, there are 4 different options here
|
Also an option would be to expose an AST parsing API as a built-in feature of Deno or as a library. From looking at #2355 it seems like this functionality will not be added to Deno but is in I think adding For |
Deno's current parser is a Rust library, and thus a bit trickier to use from JS land. Hence my suggestion of adding the necessary plumbing to Deno itself. |
I was about to write a comment about how the lint plugin option wouldn't work that well, until I realised something.
So I quickly whipped up a prototype which makes use of that to give us inline snapshots! Entire project is this, run it as per usual with Video.mp4And just the code. I'm okay with you copy-pasting and adapting this straight into Deno, and am releasing this under CC0 and MIT. import { serialize, SnapshotOptions } from "@std/testing/snapshot";
import { equal } from "@std/assert/equal";
import { AssertionError } from "@std/assert/assertion-error";
const isUpdateMode = Deno.args.some((arg) =>
arg === "--update" || arg === "-u"
);
interface SnapshotUpdateRequest {
fileName: string;
lineNumber: number;
columnNumber: number;
actual: string;
}
// Batch all writes until the very end, and then update all files at once
globalThis.addEventListener("unload", () => {
updateSnapshots();
});
function updateSnapshots() {
if (updateRequests.length === 0) {
return;
}
console.log(`Updating ${updateRequests.length} snapshots...`);
const filesToUpdate = Map.groupBy(updateRequests, (v) => v.fileName);
for (const [fileName, requests] of filesToUpdate) {
const fileContents = Deno.readTextFileSync(fileName);
const pluginRunResults = Deno.lint.runPlugin(
makeSnapshotUpdater(requests),
"dummy.ts",
fileContents,
);
const fixes = pluginRunResults.flatMap((v) => v.fix ?? []);
if (fixes.length !== requests.length) {
console.error(
"Something went wrong, not all update requests found their snapshot",
);
}
// Apply the fixes
fixes.sort((a, b) => a.range[0] - b.range[0]);
let output = "";
let lastIndex = 0;
for (const fix of fixes) {
output += fileContents.slice(lastIndex, fix.range[0]);
output += wrapForJs(fix.text ?? "");
lastIndex = fix.range[1];
}
output += fileContents.slice(lastIndex);
Deno.writeTextFileSync(fileName, output);
}
}
const updateRequests: SnapshotUpdateRequest[] = [];
export function assertInlineSnapshot<T>(
actual: T,
expected: string,
options?: SnapshotOptions<T>,
) {
const _serialize = options?.serializer ?? serialize;
const _actual = _serialize(actual);
if (equal(_actual, expected)) {
return;
}
if (isUpdateMode) {
// Uses the V8 stack trace API to get the line number where this function was called
const oldStackTrace = (Error as any).prepareStackTrace;
try {
const stackCatcher = { stack: null as SnapshotUpdateRequest | null };
(Error as any).prepareStackTrace = (
_err: unknown,
stack: unknown[],
): SnapshotUpdateRequest | null => {
const callerStackFrame = stack[0] as any;
if (callerStackFrame.isEval()) return null;
return {
fileName: callerStackFrame.getFileName(),
lineNumber: callerStackFrame.getLineNumber(),
columnNumber: callerStackFrame.getColumnNumber(),
actual: _actual,
};
};
// Capture the stack that comes after this function.
Error.captureStackTrace(stackCatcher, assertInlineSnapshot);
// Forcibly access the stack, and note it down
const request = stackCatcher.stack;
if (request !== null) {
const status = Deno.permissions.requestSync({
name: "write",
path: request.fileName,
});
if (status.state !== "granted") {
console.error(
`Please allow writing to ${request.fileName} for snapshot updating.`,
);
} else {
updateRequests.push(request);
}
}
} finally {
(Error as any).prepareStackTrace = oldStackTrace;
}
} else {
throw new AssertionError("Assertion failed blabla");
}
}
/// <reference lib="deno.unstable" />
/**
* Makes a Deno.lint plugin that can find inline snapshots.
* Also deals with multiple `assertInlineSnapshot` in a single line.
*/
function makeSnapshotUpdater(
updateRequests: SnapshotUpdateRequest[],
): Deno.lint.Plugin {
const linesToUpdate = Map.groupBy(updateRequests, (v) => v.lineNumber);
return {
name: "snapshot-updater-plugin",
rules: {
"update-snapshot": {
create(context) {
return {
'CallExpression[callee.name="assertInlineSnapshot"]'(
node: Deno.lint.CallExpression,
) {
// Find the update request that corresponds to this snapshot.
// Successful snapshots don't have an update request.
const callPosition = toLineAndColumnNumber(
node.range[0],
context.sourceCode.text,
);
const endColum = callPosition.column +
(node.range[1] - node.range[0]);
const lineUpdateRequests = linesToUpdate.get(callPosition.line);
if (lineUpdateRequests === undefined) {
return;
}
const updateRequest = lineUpdateRequests.find((v) =>
callPosition.column <= v.columnNumber &&
v.columnNumber <= endColum
);
if (updateRequest === undefined) {
return;
}
context.report({
node,
message: "",
fix(fixer) {
return fixer.replaceText(
node.arguments[1],
updateRequest.actual,
);
},
});
},
};
},
},
},
};
}
/**
* Takes an index and returns the 1-based line number and 1-based column number */
function toLineAndColumnNumber(index: number, text: string) {
const textBefore = text.slice(0, index);
// TODO: Verify that Chrome's V8 uses the same logic for returning line numbers. What about the other line terminators?
const lineBreakCount = (textBefore.match(/\n/g) || []).length;
// Also deals with the first line by making use of the -1 return value
const lastLineBreak = textBefore.lastIndexOf("\n");
return {
line: lineBreakCount + 1,
column: (index - lastLineBreak),
};
}
function wrapForJs(str: string) {
return "`" + escapeStringForJs(str) + "`";
}
function escapeStringForJs(str: string) {
return str
.replace(/\\/g, "\\\\")
.replace(/`/g, "\\`")
.replace(/\$/g, "\\$");
} |
@WWRS Would you be willing to integrate the Deno.lint approach into this PR? I'd really appreciate it, since that'd save me the effort of creating and polishing a pull request for this particular feature. If yes, here are some quick pointers
|
I'm not 100% sold on using the error throwing location as the way to find where to insert the snapshot, it seems a bit obscure and dependent on V8. As noted, we would need to double check how V8 counts line numbers, but also how it counts unicode. (For reference, Jest does use this error throwing approach.) For finding the right snapshot to update, would the numbering system that's in the current implementation of For whether or not we need a context argument, if we were to remove the argument here, we would probably want to make a similar change in I'd like to hear some other opinions on the tradeoffs of the two proposed implementations. As noted above, the magic-word implementation lacks a core capability because it requires screenshots to be manually flagged for updates. Are the missing capabilities of the lint implementation ( (It does occur to me that if we want to use this in |
I checked, the existing So I do have a working version using the lint implementation, but I'll wait to push it until we decide which we prefer. |
Thank you so much for taking a look at this. Those are some very good points, so I did some reading. Error ThrowingYes, the error throwing approach relies on V8 APIs. I wish we had a standard API for this https://github.com/tc39/proposal-error-stacks For now, I think it's fine, because updating the snapshots relies on filesystem access. So, it's limited to server-side JS runtimes, which generally implement the V8 APIs. Even Bun does. And yes, we absolutely have to check how Unicode gets counted for the column number. We'll have to verify whether the following behaves as expected. /* 🐈⬛🐈⬛🐈⬛🐈 */ assertInlineSnapshot(2 + 3, `'5'`) On that note, the linter's ranges also rely on an important detail: They're reported in UTF-16 code-somethings, which matches Javascript string indexing. So at least that part is sensible. Numbering system
(Amusingly, it means that This does not work for One quick example where the numbering system would not work is test("foo", () => {
if(false) {
assertInlineSnapshot("never happens");
}
assertInlineSnapshot("wait, why am I not getting updated");
}); I'm sure one can also think of a counterexample with promises, like calling Context argumentGood point, that'd be an odd inconsistency. So #3964 has an open issue about implementing a But clearly the normal snapshotting API needs a test context. So I looked at what Jest and Vitest do. Vitest passes the context to
Jest instead assumes that it runs within Node.js, and makes use of the I think you're better than I am at judging which of the trade-offs is the best option here.
|
I just found out that there's a proposal for async context tracking https://github.com/tc39/proposal-async-context If we had that, then we'd no longer need the first argument for the regular |
@WWRS I figured I'd check back in: How is it going? Which trade-offs should we choose? Is there anything I could do to help move this PR forward? |
6e802a8
to
12d7fd8
Compare
@kt3k This now supports AST analysis, allowing us to update by passing the @stefnotch I am now using the error throwing location instead of the numbering system. For Remaining weird stuff:
|
* the file will not be formatted, and we will throw. | ||
*/ | ||
format?: boolean; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm debating whether Pick<...>
is the most elegant option here. The alternatives would be
- Duplicating the snapshot interface. It's not that much duplication.
- Having 3 snapshot interfaces and using
extends
.
I suppose Pick
is pretty fine, the alternatives don't sound better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for all your hard work! I can't wait for this to make its way into Deno
"CallExpression"(node: Deno.lint.CallExpression) { | ||
const snapshot = locationToSnapshot[node.range[0]]; | ||
const argument = node.arguments[2]; | ||
if (snapshot === undefined || argument === undefined) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woah, I had no idea that this would end up so short and surprisingly elegant. I approve👍
) { | ||
const [lineNumber, columnNumber] = lineColumn.split(":") | ||
.map(Number); | ||
const location = (lineBreaks[lineNumber! - 2] ?? 0) + columnNumber!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why - 2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah I'll leave a comment. The line numbers are 1-indexed so we need to do a -1 to get to 0-indexed. Then we need to get the position of the break before this line, so that's the n-1
th break, which is where the second -1 comes from.
testing/_assert_inline_snapshot.ts
Outdated
} | ||
|
||
const testFilePath = fromFileUrl(this.#testFileUrl); | ||
ensureFileSync(testFilePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably left this in from snapshot.ts
. The test file should always exist, and even if it doesn't the read will error. I'll remove this.
assertEquals( | ||
await Deno.readTextFile(countTestFile), | ||
`import { assertInlineSnapshot } from "${SNAPSHOT_MODULE_URL}"; | ||
\n \r \n\r \r\n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet, thank you for testing this!
I checked the Ecmascript specification, and there are two more valid line breaking characters.
U+2028 (LINE SEPARATOR) and U+2029 (PARAGRAPH SEPARATOR)
https://tc39.es/ecma262/#sec-line-terminators
I double checked it in Chrome with this code.
eval(`a=1\u2028throw new Error("nya")`)
eval(`a=1\u2029throw new Error("nya")`)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some investigation,
const options = getOptions(msgOrOpts); | ||
const assertInlineSnapshotContext = AssertInlineSnapshotContext.fromContext( | ||
context, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a reasonable use of the test context. Am happy with this API.
If we ever want to build the expect(foo).toMatchInlineSnapshot()
API, we'll have to extract the current file name from the stack trace. But that's a future feature request that I do not absolutely need. As long as I have one API for inline snapshots, I am really happy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I believe this is ready for the Deno team to review, right? On that note, a question for @kt3k : Do you think that this is interesting enough to be noted somewhere in the Deno blog or in a Deno Tweet? While it is a major feature for me, I have no idea how widely used inline snapshots are. Hence this question. |
closes #3301
Updating the snapshots is similar to Jest: Jest uses Babel's AST parser to find where to insert the snapshot and then Prettier to format. This instead uses a Deno lint plugin to find where to insert and
deno fmt
to format.The
expectedSnapshot
s in_assert_inline_snapshot_test.ts
were automatically generated byassertInlineSnapshot
!