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.

Sign up for GitHub

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
Merged

generate a mock DataSource for use in unit testing #919

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1421b99
Add sqltest package with no-op DataSource definition
reductionista Nov 5, 2024
069f39a
sqltest -> sqlutil/sqltest
reductionista Nov 19, 2024
0e7bc8b
Merge branch 'main' into mock-datasource
reductionista Nov 20, 2024
a0dac09
Merge branch 'main' into mock-datasource
reductionista Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions pkg/sqlutil/sqltest/data_source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package sqltest

import (
"context"
"database/sql"

"github.com/jmoiron/sqlx"

"github.com/smartcontractkit/chainlink-common/pkg/sqlutil"
)

// NewNoOpDataSource returns an empty DataSource type which will satisfy the interface
func NewNoOpDataSource() sqlutil.DataSource {
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 •
edited
Loading

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.


func (ds *noOpDataSource) BindNamed(s string, _ interface{}) (string, []interface{}, error) {
return "", nil, nil
}

func (ds *noOpDataSource) DriverName() string {
return ""
}

func (ds *noOpDataSource) ExecContext(ctx context.Context, query string, args ...interface{}) (sql.Result, error) {
return nil, nil
}

func (ds *noOpDataSource) GetContext(ctx context.Context, dest interface{}, query string, args ...interface{}) error {
return nil
}

func (ds *noOpDataSource) NamedExecContext(ctx context.Context, query string, arg interface{}) (sql.Result, error) {
return nil, nil
}

func (ds *noOpDataSource) PrepareContext(ctx context.Context, query string) (*sql.Stmt, error) {
return nil, nil
}

func (_m *noOpDataSource) PrepareNamedContext(ctx context.Context, query string) (*sqlx.NamedStmt, error) {
return nil, nil
}

func (ds *noOpDataSource) QueryContext(ctx context.Context, query string, args ...interface{}) (*sql.Rows, error) {
return nil, nil
}

func (ds *noOpDataSource) QueryRowxContext(ctx context.Context, query string, args ...interface{}) *sqlx.Row {
return nil
}

func (ds *noOpDataSource) QueryxContext(ctx context.Context, query string, args ...interface{}) (*sqlx.Rows, error) {
return nil, nil
}

func (ds *noOpDataSource) Rebind(s string) string {
return ""
}

func (ds *noOpDataSource) SelectContext(ctx context.Context, dest interface{}, query string, args ...interface{}) error {
return nil
}
Loading