Skip to content

Conversation

@stefanhengl
Copy link
Member

Stacked on top of #485

Note: This adds a new dependency on autogold to src-cli

Note: This adds a new dependency on autogold to src-cli
@stefanhengl stefanhengl requested a review from keegancsmith March 5, 2021 08:58
--------------------------------------------------------------------------------
 oo bar

`
Copy link
Member

Choose a reason for hiding this comment

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

Using a gostring looks a bit ugly here. Can we rather use autogold.Raw?

@@ -1,6 +1,5 @@
github.com/Masterminds/semver v1.5.0 h1:H65muMkzWKEuNDnfl9d70GUjFniHKHRbFPGBuZ3QEww=
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised at the number of transitive dependencies autogold is adding. The use case you have here is literally just a few lines of go.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was surprised, too. I think the use case for autogold is perfect though. It is really snapshotting the UI.

@@ -0,0 +1,198 @@
package main
Copy link
Member

Choose a reason for hiding this comment

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

A different way to test this would be to start a streaming server and return your mocked events, then you can just diff against the total output of src-cli search -stream. That should cover a lot more code paths and be a test that is less likely to change as you change internal details.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Let's remove autogold then. Doing both, spinning up a stream server and using autogold is an overkill.

@stefanhengl
Copy link
Member Author

closing this one in favor of #485. Initially I wanted to use autogold for the tests, however based on the comments here I reimplemented the tests without adding new dependencies while at the same time increasing the coverage.

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.

3 participants