InsertKoinIO / koin

Koin - a pragmatic lightweight dependency injection framework for Kotlin & Kotlin Multiplatform
https://insert-koin.io
Apache License 2.0
9.06k stars 718 forks source link

Configuration check `verify()` doesn't seem to check constructor of implementation class of interface, behavior intended? #1509

Closed omtians9425 closed 1 year ago

omtians9425 commented 1 year ago

Hi, thanks for this useful library!

Describe the bug In unit tests, verify passes even if the constructor of the implementation class of the interface cannot be resolved.

To Reproduce Given the following module without a constructor DSL,

val sampleModule = module {
    single<SampleRepository> { 
        SampleRepositoryImpl(sampleService = get())
    }
    single { 
        SampleService()
    }
}

verify passes expectedly.

@Test
fun `verify sample module`() {
    sampleModule.verify()
}

However, if I remove SampleService from the module, the test still passes even though the impl class cannot be resolved.

val sampleModule = module {
    single<SampleRepository> { 
        SampleRepositoryImpl(sampleService = get())
    }
//    single { 
//        SampleService()
//    }
}

Expected behavior I would expect the test to fail when I removed SampleService.

I know the following patterns make the test fail,

// 1
val sampleModule = module {
    single {
        SampleRepositoryImpl(sampleService = get())
    }
//    single {
//        SampleService()
//    }
}

// 2
val sampleModule = module {
    singleOf(::SampleRepositoryImpl) {
        bind<SampleRepository>()
    }
//    single {
//        SampleService()
//    }
}

but if I only want to get the interface type, it becomes a problem:

Koin project used and used version (please complete the following information):

arnaudgiuliani commented 1 year ago

When you write:

single<SampleRepository> { 
        SampleRepositoryImpl(sampleService = get())
    }

Koin will only see type SampleRepository. To allow verify() function to go through your code, you need "explicit" binding:

single { 
        SampleRepositoryImpl(sampleService = get())
    } bind SampleRepository::class

or even

singleOf(::SampleRepositoryImpl) { 
       bind<SampleRepository>()
    }

The idea is to go through your Koin configurations, not scan for all class constructors.

omtians9425 commented 1 year ago

@arnaudgiuliani Thank you!

you need "explicit" binding:

Then what about Retrofit's Service interface? The implementing classes of these interfaces are not accessible from our code. Does it mean that these are not unit testable? 👀

arnaudgiuliani commented 1 year ago

Then what about Retrofit's Service interface? The implementing classes of these interfaces are not accessible from our code. Does it mean that these are not unit testable? 👀

Your retrofit interface will be injected in other class. The current version of verify() won't check if the retrofit API builder expression is ok, but that your interface is will used then.

this is Kotlin expression. Yes there is something to do 🤔

arnaudgiuliani commented 1 year ago

Verify() API is quite limited as we don't have access to reflection metadata from underlying DSL function. Koin annotations will have a better solution to fully cover all of this :/

arnaudgiuliani commented 1 year ago

For now either use checkModules or verify depending how you want to test. But those tools are quite limited in terms of type access.

pedrofsn commented 2 months ago

When you write:

single<SampleRepository> { 
        SampleRepositoryImpl(sampleService = get())
    }

Koin will only see type SampleRepository. To allow verify() function to go through your code, you need "explicit" binding:

single { 
        SampleRepositoryImpl(sampleService = get())
    } bind SampleRepository::class

or even

singleOf(::SampleRepositoryImpl) { 
       bind<SampleRepository>()
    }

The idea is to go through your Koin configurations, not scan for all class constructors.

The documentation and API doesn't warning us about the deep check not works without explicitly binding.

@arnaudgiuliani, shall we add this information in code and documentation? If yes, I could open a pull request with a suggestion, of course.