feat(testing): support override modules in test module builder

[leonardovillela]

PR Checklist

Please check if your PR fulfills the following requirements:

• [X] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• [X] Tests for the changes have been added (for bug fixes / features)
• [ ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• [ ] Bugfix
• [X] Feature
• [ ] Code style update (formatting, local variables)
• [ ] Refactoring (no functional changes, no api changes)
• [ ] Build related changes
• [ ] CI related changes
• [ ] Other... Please describe:

What is the current behavior?

Currently is not possible to override whole modules using TestingModuleBuilder.

Issue Number: https://github.com/nestjs/nest/issues/4905

What is the new behavior?

Now TestingModuleBuilder will have methods exposed in their API to override a whole module. The new method is .overrideModule used in combination with .useModule.

Does this PR introduce a breaking change?

• [ ] Yes
• [X] No

Other information

[joebowbeer]

What is the expected behavior if the new and old module do not match in global or scope?

[leonardovillela]

What is the expected behavior if the new and old module do not match in global or scope?

Hi @joebowbeer I didn't understand your question, can you provide an example please? Thanks in advance.

[leonardovillela]

All commits that I will push here trough the review will be squashed in the end, before merge the PR 😉

[micalevisk]

can you please test this feature with the following use cases:

import { Module, Injectable, DynamicModule, forwardRef } from '@nestjs/common';
import { LazyModuleLoader } from '@nestjs/core';

@Module({})
class AModule {}
@Module({})
class BModule {}
@Module({})
class CModule {}

const dynamicModule: DynamicModule = {
  module: class FooModule{},
  imports: [BModule]
}

@Module({})
class LazyModule {}
@Injectable()
export class AppService {
  constructor(private lazyModuleLoader: LazyModuleLoader) {}
  async show() {
    const moduleRef = await this.lazyModuleLoader.load(() => LazyModule)
    console.log({moduleRef})
  }
}

@Module({
  imports: [
    AModule,
    dynamicModule,
    forwardRef(() => CModule),
  ],
  providers: [AppService]
})
export class AppModule {}

• for top-level module: .useModule(AModule)
• for dynamic modules: .useModule(dynamicModule)
• for pseudo-circular dependency: .useModule(CModule)
• for nested modules: .useModule(BModule)
• for lazy-loaded modules: .useModule(LazyModule)

[leonardovillela]

can you please test this feature with the following use cases:

import { Module, Injectable, DynamicModule, forwardRef } from '@nestjs/common';
import { LazyModuleLoader } from '@nestjs/core';

@Module({})
class AModule {}
@Module({})
class BModule {}
@Module({})
class CModule {}

const dynamicModule: DynamicModule = {
  module: class FooModule{},
  imports: [BModule]
}

@Module({})
class LazyModule {}
@Injectable()
export class AppService {
  constructor(private lazyModuleLoader: LazyModuleLoader) {}
  async show() {
    const moduleRef = await this.lazyModuleLoader.load(() => LazyModule)
    console.log({moduleRef})
  }
}

@Module({
  imports: [
    AModule,
    dynamicModule,
    forwardRef(() => CModule),
  ],
  providers: [AppService]
})
export class AppModule {}

• for top-level module: .useModule(AModule)
• for dynamic modules: .useModule(dynamicModule)
• for pseudo-circular dependency: .useModule(CModule)
• for nested modules: .useModule(BModule)
• for lazy-loaded modules: .useModule(LazyModule)

@micalevisk sure, thank you for providing coding snippets to help on the test 💯 . I have only one question, the tests that you mean are just feature tests right? By using a sample folder or similar instead of writing unit or integration tests? If it's just to test this feature I even can include a new sample just for testing module overrides with all use cases that you mentioned, what do you think? 🤔

[micalevisk]

yes, I'm not talking about the automated tests.

I even can include a new sample just for testing module overrides with all use cases that you mentioned, what do you think?

that would be even better!

[joebowbeer]

@leonardovillela my question is what is supposed to happen if the replacement module has a different global setting than the module it is replacing? Similarly, what if scope settings are different?

I saw code to add the replacement module to the globals but I did not see code to remove the replaced module from globals.

[leonardovillela]

@leonardovillela my question is what is supposed to happen if the replacement module has a different global setting than the module it is replacing? Similarly, what if scope settings are different?

I saw code to add the replacement module to the globals but I did not see code to remove the replaced module from globals.

Oh I see. I'm not covering this scenario. Thanks for pointing out this, I will fix it.

[leonardovillela]

@leonardovillela my question is what is supposed to happen if the replacement module has a different global setting than the module it is replacing? Similarly, what if scope settings are different?

I saw code to add the replacement module to the globals but I did not see code to remove the replaced module from globals.

@joebowbeer the global modules now are working properly. you can run the tests on the sample added in this commit and check that.

Regarding the scope settings, I couldn't find how this is applicable to the modules themselves. The scopes are defined for controllers and providers and not at the module level.

[leonardovillela]

Hi @micalevisk, today I finally finished adding the samples and during this process. I also saw that the override was not working properly(e.g: lazy loaded modules) for some use-cases and I fixed these use-cases. Now everything should be working properly.

I could not improve more the typings and design of getOverrideModuleByModule in scanner.ts function and I'm pretty sure there is room for improvement. If you have any suggestions to improve this function, please let me know.

Can you review the samples and the commits with the fixes?

[micalevisk]

I believe you forgot to push the source codes

[leonardovillela]

I believe you forgot to push the source codes

@micalevisk sorry, now is fixed 😅😅 🤦‍♂️🤦‍♂️🤦‍♂️

[kamilmysliwiec]

Instead of adding a new sample, we should cover this functionality in the integration tests.

[leonardovillela]

Instead of adding a new sample, we should cover this functionality in the integration tests.

Done in https://github.com/nestjs/nest/pull/8777/commits/91d487032356ba61d4794a8d4cbd4fcd014f570d. @kamilmysliwiec please let me know what do you think.

[kamilmysliwiec]

Thanks @leonardovillela, I'll review it as soon as I can!

[micalevisk]

@leonardovillela from your experience on implementing this, do you mind taking a look at .overrideMiddleware proposal from https://github.com/nestjs/nest/issues/4073 I would like to know if you believe that one shouldn't be hard to tackle

Looks like we just need to find the middlewares we wan't to override on this.middleware array below

https://github.com/nestjs/nest/blob/775a34853a2ab97c38e021c96c6a4ed2de7815fb/packages/core/middleware/builder.ts#L63-L71

and then replace it on middlewareCollection Set

[leonardovillela]

@leonardovillela from your experience on implementing this, do you mind taking a look at .overrideMiddleware proposal from https://github.com/nestjs/nest/issues/4073 I would like to know if you believe that one shouldn't be hard to tackle

Looks like we just need to find the middlewares we wan't to override on this.middleware array below

https://github.com/nestjs/nest/blob/775a34853a2ab97c38e021c96c6a4ed2de7815fb/packages/core/middleware/builder.ts#L63-L71

and then replace them on middlewareCollection Set

I'm totally interested in implementing that. But since this PR is already pretty big, I think it's better to implement this feature in another PR. What do you think @micalevisk?

[micalevisk]

I think it's better to implement this feature in another PR

sure! And just write closes #4073 in PR's body so then I'll get notified about your PR :)

[leonardovillela]

@kamilmysliwiec is there something that I can do to help in the review process?

[klutzer]

@leonardovillela @kamilmysliwiec Do you have any updates on that?

[leonardovillela]

HI @klutzer, no from my side, I'm waiting review from maintainers.

[JAZZ-FROM-HELL]

This can be very useful for testing and mocking modules. +1 waiting for it.

[micalevisk]

Hi @leonardovillela could you please rebase your branch?

Also if you manage to write a sample app with your changes patched (using the patch-package tool) it will help us on trying it out

[marcelo-amorim]

Glad to see there is already a PR for this. Looking forward to it!

[flodaniel]

@leonardovillela do you think you have time finalizing the request about rebasing soon? Just came across this PR (needing it in my project) and it would be awesome if your great contribution finally reaches its destination :)