-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
snapshot testing #3635
Comments
I don't know what that means... can you explain more? |
Ref denoland/std#346 |
@ry Basically when you call The premise is that you take a snapshot of the state when things are how they should be, and if they later differ, the test fails. You also have the option to update the snapshot if you desire the change or to define a custom serializer. |
Also the option to run a specific test by name (filter) would be nice. Failfast marks all other tests as failed which is confusing. I'm finding myself commenting tests out to iterate on broken ones. |
What do you think the API should look like? I propose this: import { matchSnapshot } from 'https://deno.land/std/testing/snapshot.ts'
matchSnapshot('filename.snapshot', myObject) deno test # would fail if snapshot is outdated
deno test -u # would update snapshot if it is outdated |
I would propose the API be closer to Jest's A similar structure would be implied
|
How would a |
You could also do something like: import { makeSnapshotter } from 'https://deno.land/std/testing/snapshot.ts'
const snap = makeSnapshotter('./__snapshots__')
snap(myObject) |
@brandonkal That's what I said: "snapshot location with extra steps".
Both my design (the test file give location of the snapshot file) and yours (the test file give I think we better come up with a better design which may allow testing lib to determine snapshot file location without help from the test file, but it would require change in Deno core:
|
Just create explicit paths before you pass to matchSnapshot(new URL(import.meta.url, "testdata/filename.denosnap").pathname, myObject);
// or
matchSnapshot("__snapshots__/filename.denosnap", myObject); // from cwd Implicitly adding a directory (other than cwd) offers little control and resolving from main module seems like bad practice. |
@nayeemrmn What you proposed was my idea. However, as I also wrote in #3635 (comment), it has limitation: Deletion of redundant snapshot file is impossible (whereas in Jest, detecting outdated snapshot file is easy, because test file cannot control where to write snapshot to). |
The point is you are not supposed to have control on where the snapshot is stored. It's an implementation detail. All files have a unique import.meta.url. From there all you need is a function which maps a import.meta.url to its snapshot storage folder. It is up to the testing library where to store the snapshot. Typically it is right next to the test file so it can be committed in git. matchSnapshot("__snapshots__/filename.denosnap", myObject) That is verbose and gets messy quickly. |
@brandonkal Are we just repeating ourselves now? Yes, all If you so value the lack of control of snapshot location (just like I do), then I beg you to focus the discussion on #3635 (comment) (the second part) |
I see what you are saying now. That would be a good way to solve the issue. Just spitballing here but you could also solve this by having each test file could look like this: import { makeSnapshotter } from 'https://deno.land/std/testing/snapshot.ts'
export default function run(name) {
const snap = makeSnapshotter(name)
/// The tests
snap(myObject)
}
if (import.meta.main) run(import.meta.main) Then the import "file:///the_test.ts" to import test1 from "file:///the_test.ts"
test1("file:///the_test.ts") It's a bit more explicit but you are not injecting magic globals into the test environment. Though I wouldn't mind that either. |
But we go as far as to commit them to git, I kind of don't buy it as an implementation detail nor does automatic deletion sound so important... anyway I didn't know anything about snapshot testing before yesterday so I'll back out :p but the more I've learnt about them the more they seem like a crap jest thing. Ignore me. |
You seem to dislike snapshot. Yet even Deno itself uses something similar to snapshots ( |
Anyway, snapshot testing may or may not be in the standard library, either way is fine to me. But I think isolating every test from each other is a good idea in general, wouldn't you agree? With tests being isolated, it would then be possible for others to create a snapshot testing library that does not ask test file for |
Hardly! |
Yeah, that is like saying any assertion against a string is like snapshotting. I don't think it is a good comparison at all. I haven't voiced an opinion yet, because I am very well aware of snapshotting, but have never seen it deliver value. Developers simply rebaseline when something unexpected happens. That are, in my opinion, a false security blanket. It certainly feels like something that should start in the wider community versus being in |
I completely agree with @kitsonk on this one; it should start as third-party lib instead of I'm gonna close this issue now, but feel free to revisit if such library surfaces! |
@bartlomieju some libraries have now been created (e.g. klick) so maybe it's time to re-visit this discussion. I think it's an important feature for use cases involving SSR and SSG so Deno should consider implementing it as part of the standard library. |
Yes, I've come around on this topic, I would welcome a PR to |
That's great! 😁 As I understand it, there are currently two philosophies: The "Jest" way assertSnapshot(obj); Passing assertSnapshot(import.meta.url, obj); Currently, klicks implementation more closely resembles the "Jest" way. However, to achieve this, it wraps the Having read through the source for klick, I can't see any obvious reason why wrapping My main issue with klicks implementation is that it effectively gets determines the value of This is fine for a third party library but I'm skeptical of implementing this approach in the standard library. For a start, this is prone to breaking if the format of the stack trace changes in future. I'm very new to Deno so I know almost nothing about how the test runner works internally. It would be really useful if anyone could point me in the direction of some useful resources for better understanding the internals of the test runner - though I'm fully aware that likely no such resources exist and I just need to read the code! 😋 |
Had a quick skim through the Deno source and it looks like this file no longer exists. Seems like Deno now runs tests isolated from one another. In the spirit of spitballing, I've considered to following options for "magic globals". Option 1 - Deno global The This doesn't work because people might want to run predefined subsets of tests by importing all the test files into a parent test script like this: // <rootDir>/__test__/a.subset.test.ts
import "./a.test.ts";
import "./b.test.ts";
// <rootDir>/b.subset.test.ts
import "./__test__/a.test.ts";
import "./__test__/c.test.ts"; This is problematic because we will then end up with duplicate snapshots: // <rootDir>/__test__/__snapshots__/a.subset.test.ts.snap
// snapshots for a.test.ts
// snapshots for b.test.ts
// <rootDir>/__snapshots/b.subset.test.ts.snap`
// snapshots for a.test.ts
// snapshots for c.test.ts Furthermore if we then run just In my opinion, test snapshots should always be written to and read from the same file system path, regardless of how the test is run. Option 2 - importee.meta.url For arguments sake, lets assume we implement some way for a given script to access the URL of the file that imported it. This still wouldn't work because a common practice is to import all dependencies once from a dependencies file and re-export them. For example: // <rootDir>/deps.ts
export { assertSnapshot } from "https://deno.land/[email protected]/testing/assertSnapshot.ts";
// <rootDir>/__test__/test.ts
import { assertSnapshot } from "../deps.ts"; In this case, the snapshot would incorrectly be created in Option 3 - Maybe there's some way the The above are just my initial thoughts based on very little research and knowledge of Deno. Pleas correct me if I've gotten anything wrong! 😋 |
@bcheidemann explicitly passing |
@bartlomieju I agree, this is the best solution and also the easiest to implement. So syntax would be: // test.ts
assertSnapshot(import.meta.url, obj); // Validate snapshots (default behavior)
deno test --allow-read
// Update snapshots
deno test --allow-read --allow-write -- -u
deno test --allow-read --allow-write -- --update klick also has a "refresh" option but I think this can be added later if desirable. I would love to raise a PR if you're happy with the above? |
@bcheidemann feel free to open a PR in |
@bartlomieju usual format for snapshots is to include the test name in the snapshot file. Seems like this is why klick implements a wrapper function. As I see it, our options are:
Thoughts? |
How about using |
Sorry I missed your reply. Currently @hyp3rflow |
@bartlomieju Thanks for the explanation! But I think there is no problem with using Using assertSnapshot(import.meta.url, obj); // We don't need to provide `import.meta.url`! |
@hyp3rflow that's a good point, I'm not sure how the API would look like. I'm open to using |
@bartlomieju @hyp3rflow I would propose the following API given the above discussion: assertSnapshot(name: string, obj: any): void This would use @bartlomieju if you have no major objections to this then I will raise an initial PR that we can use as a basis for further discussion :) |
@bcheidemann please do |
@bartlomieju @bcheidemann |
@hyp3rflow @bcheidemann are you working together on this? I don't want to ask you to do duplicate work :) |
@bartlomieju No, I'm working on this feature alone. Is it okay for me to work on this? :q |
I was working on this.. But seems like I'm not anymore o.O |
I am open to working together on this. I already made a start on this and looking at your implementation @hyp3rflow I think there is good stuff that we can take from both of our approaches. Are you 100% on doing this alone? |
@bcheidemann yes I'm doing this alone and I also agree with you. |
A nice to have feature would be Jest-like snapshot testing in the std/testing library.
The text was updated successfully, but these errors were encountered: