-
Notifications
You must be signed in to change notification settings - Fork 630
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
feat(testing): Implement "assertSnapshot" #2039
Conversation
Oh.. How can I convert this PR to draft? I created this just for showing the naive implementation of |
converted to draft |
aeae7fc
to
4833857
Compare
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.
Obviously this is just a starting point so hopefully I've not been overzealous with my comments! :D
testing/asserts.ts
Outdated
const updatedSnapshotFile: Record<string, unknown> = {}; | ||
// const snapshotMap: Record<string, number> = {}; | ||
|
||
export async function assertSnapshot(context: Deno.TestContext, actual: unknown) { |
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 suggest we also expose a function called assertSnapshotSync
.
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.
Good! I think we can implement that after denoland/deno#14007 merged.
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.
It's hard to do this with the way we now read the snapshot file. I think this can probably be implemented separately if it's needed
testing/asserts.ts
Outdated
} | ||
const snapshot = snapshotFile?.[name]; | ||
if (!context.update) { | ||
assertEquals(actual, snapshot); |
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.
The current implementation will error on:
const actual = {
fn: () => { /* ... */ }
}
As snapshots need to be represented as strings anyway, I would argue it makes more sense to use the _format
helper here to convert the snapshot into a string. We could store the snapshot like this:
{
"Snapshot Test > babo > merong": [
"{",
" b: 2,",
" c: 3,",
"}"
]
}
This would enable us to catch a much wider range of errors in our snapshots. E.g. the following would error on assertSnapshot
:
// First run
const actual = {
testClass: class Test {}
}
// --snip--
assertSnapshot(test, actual)
// Second run
const actual = {
testClass: class NotTest {}
}
// --snip--
assertSnapshot(test, actual)
But would not error (or worse, always error) if we only store the JSON representation.
Additionally, this representation enables us to use the diff
and buildMessage
to produce helpful output.
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 think Deno.inspect() can be a good choice for serialization method. What do you think? @bcheidemann
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.
Yeah this would definitely do the job. I would suggest to use the _format
helper which wraps Deno.inspect
. This ensures the output will be consistent with other asserts 🙂
testing/asserts.ts
Outdated
} | ||
if (!isEqual) console.info("Snapshot updated", name); | ||
} | ||
updatedSnapshotFile[name] = actual; |
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.
Does this mean this implementation supports only 1 snapshot per test case?
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.
Does the test case mean one Deno.test
?
This implementation supports 1 snapshot per test module (one .test.ts file)
for example, if I test a.test.ts
with --update
arg,
Deno.test("Snapshot Test A", async (t) => {
await assertSnapshot(t, { a: 1, b: 2 });
await t.step("yaho", async (t) => {
await assertSnapshot(t, { b: 2, c: 3 });
await t.step("haha", async (t) => {
await assertSnapshot(t, { b: 2, c: 4 });
});
});
});
Deno.test("Snapshot Test A2", async (t) => {
await assertSnapshot(t, { a: 1, b: 2 });
await t.step("what", async (t) => {
await assertSnapshot(t, { b: 2, c: 3 });
await t.step("good", async (t) => {
await assertSnapshot(t, { b: 2, c: 4 });
});
});
});
the output, a.test.snap
will be like this
{
"Snapshot Test A": {
"a": 1,
"b": 2
},
"Snapshot Test A > yaho": {
"b": 2,
"c": 3
},
"Snapshot Test A > yaho > haha": {
"b": 2,
"c": 4
},
"Snapshot Test A2": {
"a": 1,
"b": 2
},
"Snapshot Test A2 > what": {
"b": 2,
"c": 3
},
"Snapshot Test A2 > what > good": {
"b": 2,
"c": 4
}
}
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.
How about the below case? I think the second assertSnapshot
call overwrites the snapshot from the first call
Deno.test("Snapshot Test A", async (t) => {
await assertSnapshot(t, { a: 1, b: 2 });
await assertSnapshot(t, { c: 3, d: 4 });
});
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.
Aha! Sorry, now I know what you mean. That case will be handled with commented count. (Currently not implemented perfectly so I commented) @kt3k
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, ok. I see. getCount
does that 👍
testing/asserts.ts
Outdated
@@ -882,7 +882,7 @@ export async function assertSnapshot(context: Deno.TestContext, actual: unknown) | |||
snapshotFile = file ? JSON.parse(file) : {}; | |||
} | |||
const snapshot = snapshotFile?.[name]; | |||
if (!context.update) { | |||
if (!Deno.args.includes('--update')) { |
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.
if (!Deno.args.includes('--update')) { | |
if (!Deno.args.some(arg => arg === '--update' || arg === '-u')) { |
Could do something like this to support -u
and --update
.
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 will handle this after denoland/deno#14007 merged!
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.
BTW to pass -u
option to Deno.args
, you need to invoke test command like deno test -- -u
because the flags before --
are consumed by Deno CLI itself. I'm ok with that for the first pass implementation, but it might be non ideal solution, I guess
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.
Right. I also noticed that the flag for updating snapshot can affect other codes that wanted to test.. but I think that using Deno.args
is better than making an option on Deno test cli.
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.
Imo this is the best solution for first pass. But if/when snapshot testing is a more established then we should look to implement this on the Deno CLI instead; as @hyp3rflow mentioned, this can affect test execution.
I changed the snapshot style to jest's snapshot module. |
@hyp3rflow I just landed PR in Deno repo that adds necessary fields to |
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.
This looks good! Just a couple comments but nothing major 🙂 Could you also format the file?
testing/asserts.ts
Outdated
return context.name; | ||
} | ||
function getCount() { | ||
const count = snapshotMap?.[name] ? snapshotMap[name] : 1; |
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.
const count = snapshotMap?.[name] ? snapshotMap[name] : 1; | |
const count = snapshotMap[name] ? snapshotMap[name] : 1; |
snapshotMap will never be undefined so no need 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.
Done
testing/asserts.ts
Outdated
const buf = ['export const snapshot = {};\n']; | ||
for (const [key, value] of Object.entries(updatedSnapshotFile)) { | ||
buf.push(`\nsnapshot[\`${key}\`] = \`\n${_format(value)}\n\`;\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.
This is nice because you could easily automate conversion of Jest snapshots to Deno snapshots.
However, we will need to escape backticks (`) because otherwise the following code would break:
await assertSnapshot(context, "This snapshot will break because I added a ` ");
The same will be true of the snapshot name.
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.
Fixed
testing/asserts.ts
Outdated
export async function assertSnapshot(context: Deno.TestContext, actual: unknown) { | ||
const name = getName(context); | ||
const count = getCount(); | ||
const testName = `${name} #${count}`; |
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.
const testName = `${name} #${count}`; | |
const testName = `${name} ${count}`; |
This makes it consistent with Jest snapshots, which don't have a #.
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.
Done
testing/asserts.ts
Outdated
} | ||
if (!isEqual) console.info("Snapshot updated", testName); | ||
updatedSnapshotFile[testName] = actual; | ||
globalThis.onunload = writeSnapshotFileSync; |
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.
Seems like this function will be run once per assertSnapshot
call. Can we refactor this so it only runs once?
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.
Done
testing/asserts.ts
Outdated
if (isUpdate) { | ||
let isEqual = true; | ||
try { | ||
assertEquals(_format(actual), snapshot.slice(1, -1)); |
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.
Given these values are used in both the if
and else
blocks, can we extract them out above the if
statement for better readability/maintainability e.g.
// --snip--
const _actual = _format(actual);
const _expected = snapshot.slice(1, -1);
if (isUpdate) {
// --snip--
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.
Done
testing/asserts.ts
Outdated
buf.push(`\nsnapshot[\`${key}\`] = \`\n${_format(value)}\n\`;\n`); | ||
} | ||
Deno.writeTextFileSync(snapshotPath, buf.join("")); | ||
console.log('Snapshot updated!'); |
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.
console.log('Snapshot updated!'); | |
console.log(green(bold(` > ${n} snapshots updated.`))); |
Maybe we could mimic Jest here and log how many snapshots were updated?
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.
Done
testing/asserts.ts
Outdated
} catch { | ||
isEqual = false; | ||
} | ||
if (!isEqual) console.info("Snapshot updated", testName); |
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.
if (!isEqual) console.info("Snapshot updated", testName); |
Do we need this message?
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.
Removed this for now
testing/asserts.ts
Outdated
ensureFileSync(snapshotPath); | ||
const buf = ['export const snapshot = {};\n']; | ||
for (const [key, value] of Object.entries(updatedSnapshotFile)) { | ||
buf.push(`\nsnapshot[\`${key}\`] = \`\n${_format(value)}\n\`;\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.
This doesn't need to be done now, but Jest only adds the extra \n
s when the formatted result contains a new line.
e.g.
// test file (jest)
expect("single line").toMatchSnapshot();
// snapshot file
exports[...] = `"single line"`;
// test file (jest)
expect("two\nlines").toMatchSnapshot();
// snapshot file
exports[...] = `
"two
lines"
`;
It would be nice if we could mimic this behaviour to make migration of tests easier. Again though, this can be a future improvement 😊
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.
Done
@hyp3rflow any chance we could finish this PR for next week's release? |
@bartlomieju He is absence now due to military issue (until 4/28 kst). I can fix the code on his branch, but it might be hard to finish for the next week's release. |
@bartlomieju When is next weeks release? I have a 4 day weekend so I can pick this up and should have time to finish it. I would need to create a new PR though as I don't have write access to this branch. |
@disjukr thanks for letting me know.
The release is on Wednesday, April 20th. Feel free to open up a new branch. If we miss the date, that's not a big deal though, we'll just land it the following week. |
Thanks, I'll open a new branch tomorrow 👍 |
The examples from // snap_test.ts
import { assertSnapshot } from "https://deno.land/std@$STD_VERSION/testing/snapshot.ts";
Deno.test("isSnapshotMatch", function (t): void {
const a = {
hello: "world!",
example: 123,
};
assertSnapshot(test, a);
});
I think this should be caught be some integration tests. @bcheidemann let me know if you need any help with that. This is very close to landing 🚀 |
@kt3k Good idea! 😊 I have added a test which does exactly this |
@bartlomieju I have updated the examples in What do you suggest for integration tests? EDIT: I wasn't able to reproduce the second error case ( |
This was probably caused by the absense of Now I was able to run the README example on my end (with and without |
false, | ||
); | ||
|
||
await assertSnapshot(t, formatOutput(result1.output).split("\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.
haha, nice usage of assertSnapshot
👍
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. Thank you for your contributions! @bcheidemann @hyp3rflow
const testDir = dirname(fromFileUrl(import.meta.url)); | ||
const tempDir = join(testDir, TEMP_DIR); | ||
const tempTestFileName = "test.ts"; | ||
const tempTestFilePath = join(tempDir, tempTestFileName); |
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 think it's fine for first pass, but ideally we like to use Deno.makeTempDir
for making temp directory. Filed an issue for this #2129
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.
My initial implementation did use this but I refactored as it resulted in a random output path which I would have had to obfuscate in the snapshot.
What's the advantage of using this by the way?
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.
Deno.makeTempDir
creates a directory under the system's default temp directory and they are automatically removed by the OS some time later. So we don't need care much about those files even if the test code failed to remove it. Currently we need to remove testing/.tmp
manually when the test case failed to remove it for some reason, and that's a little inconvenient
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.
Thanks for the explanation! 🙂
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! Thank you so much to @hyp3rflow and @bcheidemann, this is very useful feature and hopefully many users will find it helpful.
Thanks @bartlomieju @kt3k for all your support with this feature! |
Just noticing this now; sad to see opinionated magic around where snapshots are saved (a The simpler design also means a simpler implementation to write and maintain. Here is an example of the (IMO) ideal
Note the optional third argument |
Explicit snapshot path looks nice to me. But implicit snapshot paths are also convenient for the users who don't care about snapshot paths / snapshot file syntax. Can we somehow support both patterns by overloading |
I agree with a lot of what you're proposing but not necessarily the API. I also don't think it's too late to have a discussion around this - it's only the first pass of this feature. Why don't you raise an issue and we can continue the conversation there? 🤔 |
I don't see why not, and I have a couple of ideas how it could be done without breaking changes to the API. An issue seems like the more appropriate place to discuss this though. |
I just came back earlier due to the pandemic. Thanks, @bcheidemann for your awesome works 😊 |
Closes denoland/deno#3635
based on PR denoland/deno#14007