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

Replacing struct references with interface ones #143

Open
Szasza opened this issue Sep 4, 2020 · 5 comments
Open

Replacing struct references with interface ones #143

Szasza opened this issue Sep 4, 2020 · 5 comments

Comments

@Szasza
Copy link

Szasza commented Sep 4, 2020

Hi @guregu ,

Thank you for maintaining the package, it is much appreciated.

I would like to ask if there has been a consideration around using interface types instead of struct types to make it possible that the various parts of the package could be mocked out using gomock and mockgen.

For example if there is a method chaining such as SomeDatabase.SomeTable(...).Scan().AllWithContext(...), currently there is no easy way to provide a mock return value for AllWithContext().

The above could be solved by having interface types used instead of struct types as Scan()-like 'factory' function return types which then can be mocked out in tests to return mock object instances.

Please don't hesitate to let me know if my use-case above is unclear, I am also happy to contribute with a PR if the above fits the general direction of the package's evolution.

@guregu
Copy link
Owner

guregu commented Sep 8, 2020

Hello, thanks!
Unfortunately switching everything to an interface would be a breaking change, and there's a lot of things I'd like to change if we're going to move up a major version. It could be a good idea to switch to interfaces, or maybe even use functional parameters instead of method chaining. I've stuck with this API since 2015 so it could use a brush-up for sure. I'm using this issue to consider stuff for v2: #113, input is welcomed.
Maybe it would be possible to add an additional package like dynamodbiface in the aws-sdk? Just spitballing here.
Let me know if you have any ideas.

In the meantime I would just mock things at a higher level (for example, data repository instead of DB level), or use integration tests against dynamodb-local or a real AWS table. You could also mock the underlying dynamodbiface but I really wouldn't suggest it as you'd have to keep up with internal stuff that might break, probably painful.

Also, kind of related: I made a fake SQL driver for mocking purposes https://github.com/guregu/mogi
Maybe something like this but for dynamodbiface would be useful? Like an easy way to mock the underlying API responses. Or maybe even for dynamo instead?
guregu/mogi ended up having a number of flaws that ended up biting me later, so I tend to avoid using it, but I also think it could be useful for DynamoDB if done right.
Kind of went off on a tangent here, but I'm always open for ideas and stuff.

@Szasza
Copy link
Author

Szasza commented Sep 8, 2020

@guregu Thank you for the quick response, it is much appreciated. Sure, it is totally understandable that things are not always simple.

As for mocking, the 'mocking via data repository' way was the one I went ahead with, by creating a repo struct + interface in the app in question and injecting the mock db connection into it, which mock db was provided by https://github.com/gusaul/go-dynamock.

Similarly to mogi, potentially a built-in mocking mechanism would be even more useful and powerful for sure however based on my understanding of dynamo that seems to be a larger chunk of work than simply providing interfaces which then can be mocked using gomock. Both fits the purpose really and it is hard to guess the future use-cases.

If there was a v2.0 branch at any point in time, I would be happy to help chipping in to the implementation. Also since there is an issue already for v2.0, please feel free to close this one at any point.

Thank you again.

@kadekutama
Copy link

Hi, @Szasza and @guregu . I recently created a wrapper for this library, so it can be unit-testable. Please check it out. https://github.com/kadekutama/dynamodb

@Szasza
Copy link
Author

Szasza commented Sep 8, 2020

Hi @kadekutama. Awesome, thank you for sharing the info about the wrapper. I will give it a test run in the next few days to see if it simplifies the solution I used.

Thank you again.

@guregu
Copy link
Owner

guregu commented Oct 17, 2020

Hey @kadekutama, that's very cool! I'll add a link to your project when I get around to improving the readme.

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

No branches or pull requests

3 participants