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

Restructuring of package plus diff generation and new errors #15

Merged
merged 12 commits into from
Oct 17, 2019

Conversation

gtrevg
Copy link
Contributor

@gtrevg gtrevg commented Oct 1, 2019

This version of goldie follows the functional options pattern:
https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

It allows for easy, independent configuration of the goldie object, allowing each test case to leverage different options while running in parallel. In addition to the restructuring, a new DiffEngine option was added. This allows you to choose ClassicDiff or ColoredDiff to see the difference in the golden files, or you could even use the DiffFn option to implement your own diff function.

Errors were also updated to use pointer receivers instead of value receivers. This is the pattern that Go uses internally. In addition, the new errors package from Go v1.13 was incorporated. This will require those using this package to compile with Go v1.13 or higher.

This PR will close out #8 and #10 .

gtrevg and others added 11 commits September 25, 2019 23:25
In order allow customizations of the golden library per unit test,
the goldie library was restructured to not use global variables
and adopt the function options pattern:

  https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

This restructuring also made it easier to implement the following
features:

* Different fixture directories
* A separate directory per unit test
* A separate directory per subtest in the unit test
* Erroring out if a template didn't get data that it needed
* Diff output (classic or in color)
* Using your own diff function for output

Although a little non standard, the methods that a consumer will
be using are now all in the `interface.go` file.

In addition, error handling was cleaned up (now uses pointer
receivers) and leverages the new Go v1.13 `errors` package.

More unit testing is desired.
This description is not needed as it's already defined in goldie.go
This makes the interface.go file more purposeful and also ties similar logic
together in goldie.go
This is done to enhance readability.
[refactor] Restructuring of package plus diff generation and new errors
@gtrevg
Copy link
Contributor Author

gtrevg commented Oct 16, 2019

@sebdah Thanks for making those updates! It's now been merged into my branch.

@novas0x2a
Copy link
Contributor

Thanks for doing this! I'm just a random person who would probably use this when it lands, so take this with a grain of salt, but a few comments:

  • it looks like compareTemplate doesn't obey the diff settings
  • might be very helpful to have the filename of the golden file in the diff output (something like Expected[/path/to/template]) in the FromFile. I can override the diffEngine, obviously, but this seems like a good default

@sebdah sebdah changed the base branch from master to develop October 17, 2019 11:48
@sebdah sebdah added this to the v2.0.0 milestone Oct 17, 2019
@sebdah
Copy link
Owner

sebdah commented Oct 17, 2019

@gtrevg Thank you 👍

@sebdah
Copy link
Owner

sebdah commented Oct 17, 2019

@novas0x2a I think those are good comments, good catch on AssertWithTemplate()!

I'll address the first one in a separate PR that will be merged together with this one when we 🚢 to master.

@sebdah
Copy link
Owner

sebdah commented Oct 17, 2019

@novas0x2a Forgot to comment on the second one; Since we don't have the filename handy right now in that function, I suggest we treat that as a separate feature request. I'll create a ticket for it and we can most likely find a nifty way to address it. I think the request itself makes a lot of sense. 🙇‍♂️

@sebdah sebdah merged commit d21e92d into sebdah:develop Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants