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

Cloning injectors with registered services #6

Merged
merged 2 commits into from
Jun 25, 2022
Merged

Cloning injectors with registered services #6

merged 2 commits into from
Jun 25, 2022

Conversation

suzuki-safie
Copy link
Contributor

Cloning dependency container would be useful for unit testing.
User can reuse production service registrations and replace some service to mock service.

example:

var injector *do.Injector;
func init() {
    do.Provide[Service](injector, func (i *do.Injector) (Service, error) {
        return &RealService{}, nil
    })
    do.Provide[*App](injector, func (i *do.Injector) (*App, error) {
        return &App{i.MustInvoke[Service](i)}, nil
    })
}

func TestService(t *testing.T) {
    i := injector.Clone()
    defer i.Shutdown()
    // replace Service to MockService
    do.Provide[Service](i, func (i *do.Injector) (Service, error) {
        return &MockService{}, nil
    }))
    app := do.Invoke[*App](i)
    // do unit testing with mocked service
}

This usecase is relying on service-overriding behavior and inconsistent with #1 , so it would be nice if service-overriding behavior is officially supported.

@samber
Copy link
Owner

samber commented Jun 9, 2022

Hi @suzuki-safie and thanks for your contribution.

I like the idea of cloning+overriding injectors for tests, especially for mocks!

But I'm not really sure service overriding should be enabled by default. Such a design might lead to dangerous behavior for lazy loaded services.

I'm thinking about 2 alternatives:

  • A do.Replace[T](*do.Injector, func (*do.Injector) (T, error)) helper, similar to do.Provide
  • An option to injector constructor: do.NewWithOpts(&do.InjectorOpts{ AllowServiceOverride: true })

The second option seems better.

WDYT?

@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2022

Codecov Report

Merging #6 (5778c8a) into master (2afdbc0) will decrease coverage by 0.69%.
The diff coverage is 80.76%.

@@            Coverage Diff             @@
##           master       #6      +/-   ##
==========================================
- Coverage   91.35%   90.65%   -0.70%     
==========================================
  Files           6        6              
  Lines         370      396      +26     
==========================================
+ Hits          338      359      +21     
- Misses         24       28       +4     
- Partials        8        9       +1     
Flag Coverage Δ
unittests 90.65% <80.76%> (-0.70%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
service.go 88.88% <ø> (ø)
service_eager.go 65.21% <0.00%> (-6.22%) ⬇️
injector.go 92.57% <81.25%> (-1.14%) ⬇️
service_lazy.go 89.85% <100.00%> (+1.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2afdbc0...5778c8a. Read the comment docs.

@suzuki-safie
Copy link
Contributor Author

suzuki-safie commented Jun 9, 2022

A do.Replace[T](*do.Injector, func (*do.Injector) (T, error)) helper, similar to do.Provide

I like this option. I think do.Replace(...) expresses it's intent more clearly.

@samber
Copy link
Owner

samber commented Jun 15, 2022

Why did you close this PR ? :-(

Sorry for the late reply, I am a little bit busy. The SaaS industry is catching fire. 🔥 😬

@samber samber reopened this Jun 25, 2022
@samber samber merged commit 5b5e749 into samber:master Jun 25, 2022
@suzuki-safie
Copy link
Contributor Author

@samber
Sorry, I didn't have enough time to do this PR.
Thank you for merging!

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.

None yet

3 participants