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

[Feature Request] No method to clear hooks #1274

Open
jmaliksi opened this issue May 2, 2023 · 2 comments
Open

[Feature Request] No method to clear hooks #1274

jmaliksi opened this issue May 2, 2023 · 2 comments

Comments

@jmaliksi
Copy link

jmaliksi commented May 2, 2023

If you're having a generation problem please answer these questions before submitting your issue. Thanks!

What version of SQLBoiler are you using (sqlboiler --version)?

4.10.2

What is your database and version (eg. Postgresql 10)

Postgres 14

If this happened at generation time what was the full SQLBoiler command you used to generate your models? (if not applicable leave blank)

//go:generate sqlboiler psql --config ./sqlboiler.toml --output ./models

If this happened at runtime what code produced the issue? (if not applicable leave blank)

Hook variable declaration:

{{if or (not .Table.IsView) (.Table.ViewCapabilities.CanInsert) -}}

Add hook appends:
func Add{{$alias.UpSingular}}Hook(hookPoint boil.HookPoint, {{$alias.DownSingular}}Hook {{$alias.UpSingular}}Hook) {

These are working as documented according to the template, so just sharing the links as common point of reference

What is the output of the command above with the -d flag added to it? (Provided you are comfortable sharing this, it contains a blueprint of your schema)

n/a

Please provide a relevant database schema so we can replicate your issue (Provided you are comfortable sharing this)

n/a

Further information. What did you do, what did you expect?

The hooks are working as documented which is great; however, for testing purposes, I would like to be able to clear the hooks. The hooks we're using have service side effects, and so in the tests, I need to inject a mock that I can verify per-test case. Since the arrays are declared at the package level, and since the Add*Hook is just doing a naive append, if I configure the hooks on every test case, the arrays are going to keep growing over the course of the entire test run which is technically a memory leak. I suppose it's bound by the number of test cases so we're gonna try to move forward as is, but it's still technically a memory leak and the increasing runtime to iterate through hooks that reference finished mocks doesn't seem ideal.

Anyway so the proposal is that a Clear*Hooks be added to the template. Naively this could just clear everything, or it could take in the function pointer/identifier of the hook you wish to delete. Thinking aloud I suppose a global clear is not threadsafe for parallel tests, so probably the latter works better but also locks and whatnot might have to get added.

Happy to work on this PR if this seems like a good idea, also happy to hash this design out more.

@stephenafamo
Copy link
Collaborator

This is very tricky.
A ClearHooks method will have issues with other tests. For example, any test that runs after will have no hooks. It also means none of these tests can run in parallel, else, the hook set specifically could be cleared before the test runs

Is there a way to skip registering the service hooks at all? For example in my projects, I tend to have a hooks.Init() method to register all the hooks with any dependencies.
If you have something similar, you could skip registering them during tests, or register only the hooks without side effects.

@jmaliksi
Copy link
Author

jmaliksi commented May 3, 2023

For sure, yeah we can skip registering the hooks and that's the default behavior. They get registered in our service's main entry point on deployment and that's not called by the test suite.

However, I want to register these hooks for some tests in order to verify the side effects are occurring. Don't need them in all the tests necessarily because I'm not testing for them. For background, the side effect is a pubsub event with a specific shape and ID of model(s) affected, so it's orthogonal to our database/model stuff and other application business logic, but it's finicky enough for certain flows that I want to make sure we don't introduce regressions.

Agreed on the trickiness of parallel tests and leakage, but I think that can make it work in a generalized way with dependency injection that's also still backwards compatible. Here's a toy example of the idea we were playing with: https://go.dev/play/p/hktaMD-hqkg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants