search

tscircuit / builder

11 stars 7 forks source link

feat: Added schematicSnapshot #89

Closed imrishabh18 closed 3 months ago

imrishabh18 commented 3 months ago

Closes - https://github.com/tscircuit/tscircuit/issues/308

imrishabh18 commented 3 months ago

Yes, I am not able to get the directory in ava. Will create a __snapshots__ directory and put the files in there

seveibar commented 3 months ago

@imrishabh18 have you tried this

image

I'm mostly indifferent to the approach but having the snapshots close to the test file will help people explore outputs

imrishabh18 commented 3 months ago

Will try this, I was trying to get the directory from the fs to the place where this file which is invoking the schematicSnapshot is located. I wasn't able to get directory by that. Will give this ava methods a try

seveibar commented 3 months ago

@imrishabh18 just had another thought, we should use the extension .snapshot.png just so that people realize it's auto-generated.

Github has a really sweet image diff viewer, can't wait to see a PR that makes visual changes using this!

imrishabh18 commented 3 months ago

@seveibar What do you think now about the api?

imrishabh18 commented 3 months ago

@seveibar Great work in this format-bot. Love it how it is taking care of the format ❤️

seveibar commented 3 months ago

@imrishabh18 we want the snapshot name to always be derived from t.title, so we should default to that inside the test.

Inside a test you should call...

await snapshotSchematic(soup)

Inside get-test-fixture.ts it should do:

// schematicSnapshotOutput(t.title, soup)
writeSchematicSnapshotPng(t.title, soup)

BTW this function schematicSnapshotOutput should be renamed to be a verb and output is generally a term that can always be improved by making more specific ("data", "value" are also terms like this) schematicSnapshotOutput -> writeSchematicSnapshotPng

I'm glad __dirname works and it's totally fine but t.meta might be more reliable (maybe claude made it up?)

Otherwise looking really good

seveibar commented 3 months ago

I just noticed, the directory name is not ideal (but acceptable) IMO

image

instead of tests/__snapshots__ ideally we use t.meta.filepath (or whatever) to get it to be...

tests/path-to-test/__snapshots__/testname.snapshot.png

It's not a huge deal but if we don't do it now but want it later it'd be a bit annoying to do the migration

seveibar commented 3 months ago

@imrishabh18 this is an example of how jest snapshots are organized, ava already has builtin snapshots and they have a similar pattern where the snapshot is adjacent to the test (i like jest's more for image assets)

image

imrishabh18 commented 3 months ago

@seveibar I think this is what you were looking for. API actually looks very clean

Checked the ava codebase, the meta was not getting passed in the execution context but would have to be imported from import test from "ava" the test.