-
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
Changes from 1 commit
73467da
4d8bf6c
4833857
31abdb8
418cc43
58df39e
f191dd3
9acd82c
ea2479f
2b717b6
46b0b9a
a199814
d36765c
9df0184
2538887
c710140
c53bb73
140bd96
b6d55e3
d024982
da8e582
03b250e
f4b4174
063261b
a4bf357
6ca3bd1
5ec8e41
2f0cc55
9e4390c
fe87cfa
b1782f8
21b96d0
7ad5a8b
666f32d
3902ec7
e13af8a
158b8d5
c3264dd
e1155a0
fddf34d
24f1868
9c34b02
03d2a4c
0a66471
4542ea9
faeb691
26aba27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -13,7 +13,7 @@ import { | |||||
white, | ||||||
} from "../fmt/colors.ts"; | ||||||
import { fromFileUrl, parse, join } from "../path/mod.ts"; | ||||||
import { ensureFile } from "../fs/mod.ts"; | ||||||
import { ensureFile, ensureFileSync } from "../fs/mod.ts"; | ||||||
import { diff, DiffResult, diffstr, DiffType } from "./_diff.ts"; | ||||||
|
||||||
const CAN_NOT_DISPLAY = "[Cannot display]"; | ||||||
|
@@ -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')) { | ||||||
assertEquals(actual, snapshot); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 {
"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 // 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||||||
} else { | ||||||
let isEqual = true; | ||||||
|
@@ -895,7 +895,7 @@ export async function assertSnapshot(context: Deno.TestContext, actual: unknown) | |||||
} | ||||||
updatedSnapshotFile[name] = actual; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Does the test case mean one for example, if I test 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, {
"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 commentThe reason will be displayed to describe this comment to others. Learn more. How about the below case? I think the second 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Ah, ok. I see. |
||||||
|
||||||
Deno.test.teardown(writeSnapshotFile); | ||||||
globalThis.onunload = writeSnapshotFileSync; | ||||||
|
||||||
// function getCount() { | ||||||
// const count = snapshotMap?.[name] ? snapshotMap[name] : 1; | ||||||
|
@@ -914,10 +914,11 @@ export async function assertSnapshot(context: Deno.TestContext, actual: unknown) | |||||
return `${join(parts.dir, parts.name)}.snap`; | ||||||
} | ||||||
|
||||||
async function writeSnapshotFile() { | ||||||
function writeSnapshotFileSync() { | ||||||
const snapshotPath = getSnapshotPath(); | ||||||
await ensureFile(snapshotPath); | ||||||
ensureFileSync(snapshotPath); | ||||||
const file = JSON.stringify(updatedSnapshotFile, null, 2); | ||||||
await Deno.writeTextFile(snapshotPath, file); | ||||||
Deno.writeTextFileSync(snapshotPath, file); | ||||||
console.log('Snapshot updated!'); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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 commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,6 @@ | |
}, | ||
"Snapshot Test > babo > merong": { | ||
"b": 2, | ||
"c": 3 | ||
"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.
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 toDeno.args
, you need to invoke test command likedeno 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 guessThere 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.