Nest: [bug] 6.0.1 Dependency Injection in Testing Module

[WonderPanda]

I'm submitting a...

[ ] Regression 
[x ] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

It is not possible to inject dependencies into modules in test scenarios using the overrideProvider mechanism

Expected behavior

It should be possible to inject dependencies into modules in test scenarios using the overrideProvider mechanism

Minimal reproduction of the problem with instructions

https://github.com/WonderPanda/nest-testing-bug
Just run yarn test:e2e and you'll see the following output:

Nest can't resolve dependencies of the BugModule (?). Please make sure that the argument at index [0] is available in the BugModule context.

      at Injector.lookupComponentInExports (../node_modules/@nestjs/core/injector/injector.js:180:19)

Prior to version 6, it was simple to provide mocks for dependencies (both for other providers/controllers as well as modules) using the overrideProvider fluent methods. The change introduced here https://github.com/nestjs/nest/pull/1722/files with an additional check for this.hasProvider(toReplace) seems to have broken those scenarios. It's a very common scenario in both the libraries I've built and the tests that I've written to want to be able to supply a provider without it needing to have been previously available in the module context.

The attached repo shows a simple example of this where there is a module that requires a dependency in it's constructor:

@Module({})
export class BugModule {
  constructor(@Inject('BugProvider') private readonly bugProvider: string) {}
}

but there doesn't appear to be any mechanism now to be able to achieve this in tests.

I'm a little concerned with the initial launch of version 6 as this and the bug that was fixed as part of 6.0.1 are both core testing scenarios that have come up very often in mine and my team's use of NestJS. It seems like there's a gap right now in tests that actually validate that the TestingModule works properly and I'm hesitant to suggest people try version 6 until this is addressed.

[kamilmysliwiec]

Well, this was never meant to be used in this way. Basically, you shouldn't be able to provide things from outside of modules using overrideProvider. This method has been added to allow replacing providers. Unfortunately, it had a bug (which now is fixed) that instead of overriding providers by passed token, was adding it to every module (which means that they were actually replaced as well, but still). We definitely shouldn't revert back to the previous behavior, because it had major gaps (useClass and useFactory broke whole apps due to the lack of dependencies in context).

Regarding your requirement, instead of using overrideProvider, you should rather add a static method that would allow to configure the module dynamically. Modules are supposed to isolate things inside, but you can expose "configuration" interface using dynamic modules.

[WonderPanda]

Thanks for the clarification @kamilmysliwiec, was just unfortunate because I had a bunch of code from 5.7 that depended on this functionality. I do expose a static build method from the module in question which I will try to leverage to see if I can get around this change. I think that it will just make the test setup code a bit more verbose. I think that there might be some value in having some test shortcuts to be able to add providers in this fashion as it could cut down on a lot of boilerplate.

Closing this as it is now the expected behavior and will find a work around for my current scenario

[kamilmysliwiec]

We can add a helper method that would allow adding provider to certain module scope. PRs are open if you are interested :)

[romandecker]

@kamilmysliwiec could you elaborate on the current best practice for 6.0? We're in the situation that' we'd like to overrideProvider a globally provided Config-injectable. This does no longer work with v6. We could refactor the tests to just list { provide: Config, useValue: mockConfig } in the providers of the testing-module, instead of using overrideProvider like we used to.

Is this still viable?

[kamilmysliwiec]

What do you mean by:

globally provided Config-injectable

this? Module with @Global decorator?

[romandecker]

Yes, this is how we define our ConfigModule:

import { DynamicModule, Global, Module } from '@nestjs/common';
import { configFactory } from './env-config.factory';
import { EnvConfigModuleOptions } from './interfaces';

@Global()
@Module({})
export class EnvConfigModule {
    static forRoot(options?: EnvConfigModuleOptions<any>): DynamicModule {
        const providers = [
            {
                provide: options.schema,
                useValue: configFactory(options),
            },
        ];

        return {
            module: EnvConfigModule,
            providers,
            exports: providers,
        };
    }
}

It dynamically provides an injectable of type options.schema globally - this is how we use it:

export class Config {
    @IsInt()
    PORT: number = 3000;
}

@Module({
    imports: [
        EnvConfigModule.forRoot({
            schema: Config,
            baseConfigPath: path.resolve(__dirname, '../config'),
        })
    ],
    /* ... */,
})
export class AppModule {}

Our config module reads configuration values, does validation and puts the resulting config into an instance of Config, which is then globally available across the app, because the EnvConfigModule is annotated with @Global().

The problem is that in our tests, we used to just use overrideProvider(Config, mockConfig) to mock it. This breaks with v6.

[kamilmysliwiec]

Could you please provide a small repo so I can point you at the right direction? :)

[romandecker]

The code is spread across multiple packages, I will try to provide a minimal example, illustrating the case, will take some time though :/

[kamilmysliwiec]

Got it. Can't you just import EnvConfigModule in your testing module? (and provide testing mockConfig through forRoot())?

[romandecker]

I guess I could but it would no longer be a unit test - the configuration logic is tested independently somewhere else already, I don't want to import that into all my tests.

For unit tests, I want to provide my class under test with exactly the dependencies it needs, and not have to bring in additional business logic just to be able to build a mock.

For integration tests, this can be a more viable approach (and it's actually kind of what we're already doing there, in a way).

[kamilmysliwiec]

If you rely on Config provider used through the overrideProvider that is no longer unit test either.

[apiel]

In your example for unit testing https://docs.nestjs.com/fundamentals/testing#testing-utilities you are using Test.createTestingModule

What is then the advantage to use Test.createTestingModule over:

    catsService = new CatsService();
    catsController = new CatsController(catsService);

?

I would completely understand to use Test.createTestingModule for integration test, to see if everything work well together, but I don't really see the value in unit test.

[kamilmysliwiec]

https://docs.nestjs.com/fundamentals/testing#unit-testing

We didn't make use of any existing Nest testing utility so far. Since we have manually taken care of instantiating tested classes, above test suite has nothing to do with Nest. This type of testing is called isolated tests.

These are two different types of tests. In the most cases, you won't need Test.createTestingModule.

[apiel]

Maybe the documentation is misleading. My first approach for our unit test (isolated tests) was to use catsController = new CatsController(catsService); and not Test.createTestingModule. But because of the documentation, my team convinced me that we should use Test.createTestingModule and I am always in favor to follow the doc.
I guess, we need to clarify the doc to make this more understandable. Maybe you could move the Testing utilities section inside of End-to-end testing. Or if you want to keep it after the Unit testing section, you could put a hint "In the most cases, you won't need Test.createTestingModule for unit testing". Or something like that, but yeah, writing docs is always super difficult -.-

[kamilmysliwiec]

I'll provide more detailed information shortly :)

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.