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

Mocking capabilities #106

Closed
gionapaolini opened this issue Feb 17, 2022 · 8 comments
Closed

Mocking capabilities #106

gionapaolini opened this issue Feb 17, 2022 · 8 comments

Comments

@gionapaolini
Copy link

I am writing some unit tests where I need to mock esdb.Client.AppendToStream and esdb.Client.ReadStream

I have created an interface to mock the client:

type IEventStoreClient interface {
	AppendToStream(context context.Context, streamID string, opts esdb.AppendToStreamOptions, events ...esdb.EventData) (*esdb.WriteResult, error)
	ReadStream(context context.Context, streamID string, opts esdb.ReadStreamOptions, count uint64) (*esdb.ReadStream, error)
}

Now while I don't have issue with AppendToStream, since I don't need to use the returned *esdb.WriteResult, mocking esdb.Client.ReadStream becomes very complicated since it is not an interface, and the actual logic within ReadStream.Recv() is the part that needs mocking.

Wouldn't it be preferable to use an interface (IReadStream or something)?

@oskardudycz
Copy link
Contributor

oskardudycz commented Feb 18, 2022

@gionapaolini, could you explain more of your use case? I'd like to understand why you need to mock the reading result?

Our general recommendation is to split the responsibility between the domain/application code and infrastructure part. By having that, you can unit test your business logic, then have a set of integration tests that will ensure that all of the components joined together work as expected. A potential solution is to use Repository Pattern, see an excellent article on that topic from @roblaszczak: https://threedots.tech/post/repository-pattern-in-go/.

Also, for instance: If you're doing Event Sourcing, then typically, you'd like to get the current state from events, so instead of returning EventStoreDB class, you could return the state.

We don't recommend mocking the client classes. Besides what I wrote above, the appending and reading logic from the big picture perspective may look simple and easy to mock. However, the implementation details like connection management, serialisation, clustering, etc., can cause (in edge cases) a significant difference in the final implementation. In the worst case, it may end up in a situation where all unit tests are green, but after deployment, features are failing.

We provide a Docker image (see more in our docs: https://developers.eventstore.com/server/v21.10/installation.html#docker) that you could use in your CI. It is relatively lightweight and shouldn't be a considerable overhead. You can consider using TestContainers, which makes the test setup easier (see https://golang.testcontainers.org/).

If I didn't cover your use case, feel free to expand on that, then I could try to give further suggestions.

@gionapaolini
Copy link
Author

Hi @oskardudycz , thanks for the detailed answer.

Please let me know if I misunderstood something, your main concerns are that:

  • the domain layer is not decoupled enough from the application layer
  • mocking the client would hide possible failure due to the real implementation

To address the first point, I am already using a repository pattern:

I have an interface for the repository, which has a Save(menu Menu) error and Get(id uuid.UUID) (Menu, error) methods.
This interface is used in the domain logic.
Then I have an implementation specific to the EventStore, that implements the Save and Get methods.

Here you can see the implementation for the GetMenu:

func (eventStore MenuEventStore) GetMenu(id uuid.UUID) (aggregates.Menu, error) {
	menu := aggregates.Menu{}
	streamName := "Menu-" + id.String()
	options := esdb.ReadStreamOptions{}
	stream, err := eventStore.db.ReadStream(context.Background(), streamName, options, 200)
	if esdbError, ok := esdb.FromError(err); !ok {
		if esdbError.Code() == esdb.ErrorResourceNotFound {
			return aggregates.Menu{}, ErrMenuNotFound
		}
	}
	defer stream.Close()
	for {
		eventData, err := stream.Recv()
		if errors.Is(err, io.EOF) {
			break
		}
		if err != nil {
			return aggregates.Menu{}, err
		}
		event := aggregates.DeserializeMenuEvent(eventData.Event.Data)
		menu = menu.AddEvent(event)
	}
	return menu, nil
}

Currently the logic to re-construct the Menu is included in the GetMenu, which I understand it's debatable (I could just return all the events and delegate the reconstruction to another object, maybe a factory/builder), but even without the reconstruction part, it is worth to be covered by a unit test since there is more than only client calls.

To address the second point, I understand the implementation of the client is complex, but that is exactly why I want to mock it.
I want to test my code, not the actual eventstore client implementation (for which I agree integration tests will be needed).
Here I'm focusing on writing unit tests and they should be fast and with no external dependencies.

Let me know if something is still unclear or you have further suggestions for my case

@ylorph
Copy link

ylorph commented Feb 18, 2022

In the code you showed, testing the logic of reconstructing a menu ( menu.AddEvent(event) ) can be done in specific tests, and no connection is required for that

What I'd certainly delegate somewhere else is the stream name ( either a convention or some code handing out the stream name for the specific entity type )
That also can be covered by test not requiring a connection

what is then left to test are infrastructure code concerns, that are better tested against a real connection than a mock .

@ylorph ylorph linked a pull request Feb 18, 2022 that will close this issue
@gionapaolini
Copy link
Author

gionapaolini commented Feb 18, 2022

Hi @ylorph thank you for your input.

I don't quite understand why it's preferable to test with a real connection.

The objective of a unit test is to ensure the logic of my code works as expected, not the logic behind the client. I want to ensure that the client gets called correctly with some specific parameters, and that the results are correctly processed.

Even if we strip out all the rest, we are left with quite some logic that I need to cover.

func (eventStore MenuEventStore) GetEvents(streamName string) ([]*esdb.ResolvedEvent, error) {
	options := esdb.ReadStreamOptions{}
	stream, err := eventStore.db.ReadStream(context.Background(), streamName, options, 200)
	if esdbError, ok := esdb.FromError(err); !ok {
		if esdbError.Code() == esdb.ErrorResourceNotFound {
			return []*esdb.ResolvedEvent{}, ErrMenuNotFound
		}
	}
	defer stream.Close()
        events := []*esdb.ResolvedEvent{}
	for {
		eventData, err := stream.Recv()
		if errors.Is(err, io.EOF) {
			break
		}
		if err != nil {
			return []*esdb.ResolvedEvent{}, err
		}
		events = append(events, eventData)
	}
	return events, nil
}

I understand I can test directly with a real connection, but the running time of the test grows a lot when doing so (from 0.003s to 0.1, for a test that saves and gets). And if we follow this principle, then we would (almost) never need Mocking.

I am not saying it should not be tested with a real connection, just that I think we are talking about different scopes (unit vs integration test)


Edit: One specific case I can think of is, what if I want to test how the app behave in case of issues with the eventstore (connection errors, etc..) with a mock I can easily simulate that

@ylorph
Copy link

ylorph commented Feb 18, 2022

unit vs integration test

Indeed,
the logic of events = append(events, eventData) is unit tests..
anything that implies a connection is integration

with a mock I can easily simulate that

You can't simulate all the possible way the connection / server / network in between will fail.

Additionally, I know of no database out there that provide interface to their connection for mocking reason, because of my above statement.

@gionapaolini
Copy link
Author

I wasn't aware of all the online discussions on the topic (to mock or not to mock)
I am not convinced yet, but I just quickly read few things and I guess I will need to go a bit deeper to make an informed decision.

In the meantime thank you for the info

@oskardudycz
Copy link
Contributor

@gionapaolini, I agree with @ylorph. We're trying to say here that the unit testing mocked interface would not give you the guarantees you (probably) expect to get from the test. Such a test would be more testing the implementation rather than actual functionality. By that, I mean it will check the coding mechanics if you call the specific set of methods.

As I explained in my initial answer, the implementation specifics may differ a lot between the mock assumptions and what will happen in the actual database connection. Thus, some scenarios are great for unit testing, like business logic, etc. However, it's better to test using real tools (e.g. databases). Indeed the performance may be a bit lower, but then you're getting much better certainty that you're code is doing what you expect it to do. I think that such confidence is much more important than cutting some milliseconds in the test run.

Of course, that requires getting the right balance. I'm not suggesting testing all the code with integration tests. You could test the method that you pasted with the set of integration tests to make sure that it's working as expected. Then the code using it can trust that set of tests, don't repeat them and use mocks for your interface. The most important thing for me is to ask myself what this test is trying to verify and check. If it's just checking if I expectedly called some method, then that's not the most crucial test for me, as it's checking how I code, not how the stuff works.

I wrote recently on a similar topic in my article https://event-driven.io/en/i_tested_on_production/.

Check also:

@oskardudycz
Copy link
Contributor

@gionapaolini, because of the explanation I gave above, I will close the issue and pull the request. If you still want to continue the discussion, I could clarify or suggest something more; feel free to send the feedback.

Even though this is not the change that we'd like to introduce, I'd like to thank you for your contribution. We're open to the further one. I'd suggest starting with posting the issue and discussing the details of the potential code contribution.

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 a pull request may close this issue.

3 participants