Skip to content
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

chore: use snapshot tests #228

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

JamesGuthrie
Copy link
Member

No description provided.

@JamesGuthrie JamesGuthrie requested a review from a team as a code owner November 15, 2024 16:13
@JamesGuthrie JamesGuthrie force-pushed the jg/use-snapshot-tests branch 5 times, most recently from 51550bd to ec32a46 Compare November 15, 2024 16:44
Uses `syrupy` to handle snapshotting of text outputs. To update the
snapshot files, run `pytest --snapshot-update` or `just test-update`.
Copy link
Collaborator

@jgpruitt jgpruitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not crazy about this change. I like that it reduces code, however it feels like a worse experience.

There's no typing or even an import statement to clue somebody into what snapshot is or how it's supposed to work. You have to know to go look at syrupy and learn how it works. Previously, there was more code, but it was straightforward.

The actual output isn't stored anywhere on disk where I can look at it easily with whatever tool I wish.

Correct me if I'm wrong, but just test-update is all-or-nothing. When dealing with changes that trigger these test failures, I prefer to run each in isolation and carefully fix each one at a time. There's less mental load that way.

text=True,
capture_output=True,
)
return result.stdout
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return result.stdout
return str(result.stdout)

and change the function's return type to str

text=True,
capture_output=True,
)
return result.stdout
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return result.stdout
return str(result.stdout)

and change the return type to str

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants