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

generate a mock DataSource for use in unit testing #919

Merged
merged 4 commits into from
Nov 20, 2024

Conversation

reductionista
Copy link
Contributor

@reductionista reductionista commented Nov 6, 2024

This seems like a good thing to have in general, I just happened to find it useful for the two linked PR's below, where you can see an example of it being used. In this case, an empty interface would have been good enough, but having it backed by an actual in-memory db allows us to do a few more things with it and should make it behave a bit more like a realistic mock. What won't work for now is setting up the tables and fixtures chainlink components usually expect, so it would only be good for cases where not much is done with the data source. We'll still need txdb for most things which is moved into chainlink-common in another set of linked PR's.

Supports

solana ref: dce6483e2bd7bde0eb564c5e3ed703186c232f90
core ref: 3ad3e7514fc86a3a3935162e3cba67a410858dde

@jmank88
Copy link
Collaborator

jmank88 commented Nov 6, 2024

We have been trying to avoid sharing mocks between modules because it splinters the version of mockery being used in the main module.

@jmank88
Copy link
Collaborator

jmank88 commented Nov 6, 2024

On the other hand, if we wanted to provide some kind of "fake" in-memory implementation, then that could live here in a test package.

@reductionista
Copy link
Contributor Author

We have been trying to avoid sharing mocks between modules because it splinters the version of mockery being used in the main module.

That seems like a big self-imposed limitation to have. Is there no way to keep the mockery versions in sync between these repos?

@reductionista
Copy link
Contributor Author

reductionista commented Nov 6, 2024

On the other hand, if we wanted to provide some kind of "fake" in-memory implementation, then that could live here in a test package.

That's a good idea. For now, the only thing I need is anything that satisfies the interface. So I could just commit it with unimplemented methods that return nil. Although having an actual in-memory version of a postgres data source for testing would certainly be very useful for what's coming up... so I'll take a look and see if that's something I can easily add.

@jmank88
Copy link
Collaborator

jmank88 commented Nov 6, 2024

We have been trying to avoid sharing mocks between modules because it splinters the version of mockery being used in the main module.

That seems like a big self-imposed limitation to have. Is there no way to keep the mockery versions in sync between these repos?

There is no limitation. Anybody that wants to use a mock is free to generate one for themselves.

@reductionista
Copy link
Contributor Author

There is no limitation. Anybody that wants to use a mock is free to generate one for themselves.

Not sure what you mean by this. When the structure you need to mock is in chainlink-common, you're not supposed to generate the mock in chainlink-common. So, are you saying it should be generated separately by each thing importing it? Would the generation be automatic, or just done manually and copied over?

@jmank88
Copy link
Collaborator

jmank88 commented Nov 8, 2024

There is no limitation. Anybody that wants to use a mock is free to generate one for themselves.

Not sure what you mean by this. When the structure you need to mock is in chainlink-common, you're not supposed to generate the mock in chainlink-common. So, are you saying it should be generated separately by each thing importing it? Would the generation be automatic, or just done manually and copied over?

https://github.com/smartcontractkit/chainlink/blob/develop/.mockery.yaml#L343-L350

pkg/sqltest/data_source.go Outdated Show resolved Hide resolved
return &noOpDataSource{}
}

type noOpDataSource struct{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, is it clear why you cannot use dataSource from pkg/sqlutil/hook_test.go instead of noOpDataSource ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could combine them but this version does a little more. I suppose this one could embed the new one and then just override the two modified methods 🤷

Copy link
Contributor

@pavel-raykov pavel-raykov Nov 20, 2024

Choose a reason for hiding this comment

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

From what I understand noOpDataSource is intended to be used in some tests. Would dataSource from pkg/sqlutil/hook_test.go just work as it is now? (Or alternatively, one could also call this HookableDataSource that can be initialized with hookBehaviour = false / true [this is just another way to implement the desire behavior without embedding])

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would dataSource from pkg/sqlutil/hook_test.go just work as it is now?

I don't think that an exported "no op" type should be arbitrarily sleeping for any amount of time.

(Or alternatively, one could also call this HookableDataSource that can be initialized with hookBehaviour = false / true [this is just another way to implement the desire behavior without embedding])

Can you elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would dataSource from pkg/sqlutil/hook_test.go just work as it is now?

I don't think that an exported "no op" type should be arbitrarily sleeping for any amount of time.

I agree, but it would sleep only if GetContext/SelectContext is called - I am not sure how often this would happen.

(Or alternatively, one could also call this HookableDataSource that can be initialized with hookBehaviour = false / true [this is just another way to implement the desire behavior without embedding])

Can you elaborate?

Instead of embedding the common functions and having two structs HookedDataSource and NoOpDataSource, one could have one HookableDataSource that would behave like HookedDataSource if initialized with true and like NoOpDataSource if initialized with false.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of embedding the common functions and having two structs HookedDataSource and NoOpDataSource, one could have one HookableDataSource that would behave like HookedDataSource if initialized with true and like NoOpDataSource if initialized with false.

If there is a way to expose a general type that can be customized to do exactly what the current hook type is doing, then that would be great. But I don't think it would make sense to put this specialized behavior behind a flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, do you mean that something like

type dataSource struct {
	hookable bool
}

func (q *dataSource) SelectContext(ctx context.Context, dest interface{}, query string, args ...interface{}) error {
	if q.hookable {
		select {
		case <-ctx.Done():
			return ctx.Err()
		case <-time.After(selDur):
		}
	}
	return nil
}

is not good?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that behavior is arbitrary in the context of a general exported type. It is just what one particular set of tests needed at the time, and there is no reason to assume that any other users would want the same behavior (in principle or with those particular const durations).

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, I am just trying to find a way to avoid code duplication. I would also be fine with the embed solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pavel-raykov I appreciate the suggestion, but could I ask that you raise a separate PR for refactoring that other function after this gets merged? I've been trying to get this one merged for 2 weeks now, and each time I change it there's a whole chain of other PR's it's holding up that have to have their refs updated.

@reductionista reductionista merged commit 262c6d8 into main Nov 20, 2024
10 of 11 checks passed
@reductionista reductionista deleted the mock-datasource branch November 20, 2024 19:06
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.

4 participants