abedra / libvault

A C++ library for Hashicorp Vault
MIT License
34 stars 25 forks source link

marking VaultClient and HttpClient public methods as virtual #25

Closed waybeforenow closed 4 years ago

waybeforenow commented 4 years ago

In order to create mock instances of VaultClient and HttpClient (which you may want to do for testing, at some point), their methods need to be marked virtual so we can define override functions for them later.

You may want to do this for other classes at some point, but these two seem like the ideal place to start since HttpClient is tightly coupled to an object with real side effects (CURL) and is not dependency injectable into VaultClient.

abedra commented 4 years ago

This seems reasonable to me. Can you add a couple tests that demonstrate the override option so that others can better understand that testing strategy?

waybeforenow commented 4 years ago

I've been hung up on this a bit because I don't know what needs better coverage, so I don't know what tests to write, but I added a couple of mock classes so that the response from HttpClient can be mocked. Is that good? Do you have any suggestions about how this can improve the test coverage?

The repo that has the tests where I'm mocking VaultClient and HttpClient is private, but I can share the code with you if you're curious. It uses googletest and googlemock.

abedra commented 4 years ago

My goal in having a test that demonstrates this is not necessarily to find things that aren't well covered, but to demonstrate the idea. Even if it's just a test for a key value get or something like that it will show the end to end process as well as make sure there aren't any other ideas that need to be addressed.

You can run the example and get the responses if you want to run with a "fixture" style mock.

waybeforenow commented 4 years ago

I added test coverage for the mocks.

abedra commented 4 years ago

Excellent, thank you