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

leonardovillela · 2021-12-12T22:49:45Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
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?

What is the current behavior?

Currently is not possible to override whole modules using TestingModuleBuilder.

Issue Number: #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
No

Other information

micalevisk · 2021-12-12T22:54:39Z

packages/core/injector/container.ts

+    await this.addDynamicMetadata(
+      token,
+      dynamicMetadata,
+      [].concat(scope, type),


I'm curious. Why did you use [].concat(scope, type) instead of [scope, type]?

No specific reason, I just followed the same approach as the one in addModule. In fact, I think would be better If I refactor that common logic to replaceModule and addModule in a private method. What do you think?

oh I didn't saw that addModule

I don't think it will improve for now, actually. To me, it is better to keep this duplicated as it will help future refactoring.

unless you manage to abstract this piece of logic:

const moduleRef = new Module(type, this); moduleRef.token = token; this.modules.set(token, moduleRef); await this.addDynamicMetadata( token, dynamicMetadata, [].concat(scope, type), ); if (this.isGlobalModule(type, dynamicMetadata)) { this.addGlobalModule(moduleRef); } return moduleRef;

if you could, then I guess you can change that to [scope, type]

unless you manage to abstract this piece of logic:

const moduleRef = new Module(type, this); moduleRef.token = token; this.modules.set(token, moduleRef); await this.addDynamicMetadata( token, dynamicMetadata, [].concat(scope, type), ); if (this.isGlobalModule(type, dynamicMetadata)) { this.addGlobalModule(moduleRef); } return moduleRef;

Was exactly about this one that I was talking. I will try to refactor it and update this PR. Thanks for the feedback =)

Done in 3114a54. Can you please re-review @micalevisk? I also opted to keep the [].concat(scope, type), because the equivalent operation in fact would be [...scope, type], I didn't see much value of refactoring that, because both sound very much the same thing for me, so I opted to not change.

oh, right. I didn't notice that scope is an array. I agree with you

joebowbeer · 2021-12-13T05:57:33Z

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

leonardovillela · 2021-12-13T19:12:46Z

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 · 2021-12-13T19:42:17Z

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

micalevisk · 2021-12-13T20:38:29Z

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 · 2021-12-13T21:15:39Z

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 · 2021-12-13T21:19:25Z

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 · 2021-12-13T22:43:45Z

@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 · 2021-12-15T19:13:44Z

@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 · 2021-12-19T16:08:47Z

@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 · 2021-12-19T16:45:10Z

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 · 2021-12-19T17:23:32Z

I believe you forgot to push the source codes

leonardovillela · 2021-12-19T18:35:05Z

I believe you forgot to push the source codes

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

kamilmysliwiec · 2021-12-20T09:05:52Z

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

leonardovillela · 2021-12-20T23:19:55Z

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

Done in 91d4870. @kamilmysliwiec please let me know what do you think.

kamilmysliwiec · 2021-12-21T09:35:09Z

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

micalevisk · 2021-12-22T02:08:44Z

@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

nest/packages/core/middleware/builder.ts

Lines 63 to 71 in 775a348

    
           const configuration = { 
        
             middleware: filterMiddleware( 
        
               this.middleware, 
        
               this.excludedRoutes, 
        
               this.builder.getHttpAdapter(), 
        
             ), 
        
             forRoutes, 
        
           }; 
        
           middlewareCollection.add(configuration);

and then replace it on middlewareCollection Set

leonardovillela · 2021-12-22T21:48:34Z

@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

nest/packages/core/middleware/builder.ts

Lines 63 to 71 in 775a348

const configuration = {

middleware: filterMiddleware(

this.middleware,

this.excludedRoutes,

this.builder.getHttpAdapter(),

),

forRoutes,

};

middlewareCollection.add(configuration);

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 · 2021-12-22T21:52:30Z

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 · 2022-01-08T13:46:58Z

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

klutzer · 2022-08-16T18:39:22Z

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

leonardovillela · 2022-08-17T11:51:36Z

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

JAZZ-FROM-HELL · 2022-09-29T09:30:08Z

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

micalevisk · 2022-10-05T02:47:06Z

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 · 2023-01-09T22:13:34Z

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

flodaniel · 2023-04-13T06:17:20Z

@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 :)

closes: nestjs#8777

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

2071461

leonardovillela mentioned this pull request Dec 12, 2021

TestingModuleBuilder is missing overrideModule #4905

Closed

micalevisk reviewed Dec 12, 2021

View reviewed changes

refactor(testing): remove duplication to add/replace container module

3114a54

feat(testing): add missing validation for replace container module

6ea534e

leonardovillela added 3 commits December 19, 2021 00:02

fix(testing): circular dependency module override

d7351b7

fix(testing): lazy module override

59bae05

chore(testing): remove debugger instructions

09f6c94

docs(testing): add modules override sample

d725aae

leonardovillela force-pushed the testing-module-builder-add-override-module-method branch from f2f8213 to d725aae Compare December 19, 2021 18:33

refactor(testing): transform module override sample to integration test

91d4870

kamilmysliwiec added type: feature 🎉 scope: testing labels May 13, 2022

kamilmysliwiec changed the base branch from master to 10.0.0 April 17, 2023 12:05

kamilmysliwiec added this to the 10.0.0 milestone Apr 17, 2023

kamilmysliwiec merged commit fdfabf9 into nestjs:10.0.0 Apr 17, 2023

kamilmysliwiec mentioned this pull request Apr 17, 2023

chore: laying the grounds for 10.0.0 #11517

Merged

12 tasks

schiemon added a commit to schiemon/nest that referenced this pull request Oct 9, 2023

feat(core,common,testing): support overriding middleware for testing

334dff7

closes: nestjs#8777

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

leonardovillela commented Dec 12, 2021

micalevisk Dec 12, 2021

leonardovillela Dec 13, 2021

micalevisk Dec 13, 2021 •

edited

Loading

micalevisk Dec 13, 2021

micalevisk Dec 13, 2021

leonardovillela Dec 13, 2021 •

edited

Loading

leonardovillela Dec 13, 2021 •

edited

Loading

micalevisk Dec 13, 2021

joebowbeer commented Dec 13, 2021

leonardovillela commented Dec 13, 2021

leonardovillela commented Dec 13, 2021 •

edited

Loading

micalevisk commented Dec 13, 2021

leonardovillela commented Dec 13, 2021

micalevisk commented Dec 13, 2021 •

edited

Loading

joebowbeer commented Dec 13, 2021

leonardovillela commented Dec 15, 2021

leonardovillela commented Dec 19, 2021

leonardovillela commented Dec 19, 2021

micalevisk commented Dec 19, 2021

leonardovillela commented Dec 19, 2021 •

edited

Loading

kamilmysliwiec commented Dec 20, 2021

leonardovillela commented Dec 20, 2021

kamilmysliwiec commented Dec 21, 2021

micalevisk commented Dec 22, 2021 •

edited

Loading

leonardovillela commented Dec 22, 2021

micalevisk commented Dec 22, 2021

leonardovillela commented Jan 8, 2022 •

edited

Loading

klutzer commented Aug 16, 2022

leonardovillela commented Aug 17, 2022 •

edited

Loading

JAZZ-FROM-HELL commented Sep 29, 2022

micalevisk commented Oct 5, 2022 •

edited

Loading

marcelo-amorim commented Jan 9, 2023

flodaniel commented Apr 13, 2023

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

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

Conversation

leonardovillela commented Dec 12, 2021

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

micalevisk Dec 12, 2021

Choose a reason for hiding this comment

leonardovillela Dec 13, 2021

Choose a reason for hiding this comment

micalevisk Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

micalevisk Dec 13, 2021

Choose a reason for hiding this comment

micalevisk Dec 13, 2021

Choose a reason for hiding this comment

leonardovillela Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

leonardovillela Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

micalevisk Dec 13, 2021

Choose a reason for hiding this comment

joebowbeer commented Dec 13, 2021

leonardovillela commented Dec 13, 2021

leonardovillela commented Dec 13, 2021 • edited Loading

micalevisk commented Dec 13, 2021

leonardovillela commented Dec 13, 2021

micalevisk commented Dec 13, 2021 • edited Loading

joebowbeer commented Dec 13, 2021

leonardovillela commented Dec 15, 2021

leonardovillela commented Dec 19, 2021

leonardovillela commented Dec 19, 2021

micalevisk commented Dec 19, 2021

leonardovillela commented Dec 19, 2021 • edited Loading

kamilmysliwiec commented Dec 20, 2021

leonardovillela commented Dec 20, 2021

kamilmysliwiec commented Dec 21, 2021

micalevisk commented Dec 22, 2021 • edited Loading

leonardovillela commented Dec 22, 2021

micalevisk commented Dec 22, 2021

leonardovillela commented Jan 8, 2022 • edited Loading

klutzer commented Aug 16, 2022

leonardovillela commented Aug 17, 2022 • edited Loading

JAZZ-FROM-HELL commented Sep 29, 2022

micalevisk commented Oct 5, 2022 • edited Loading

marcelo-amorim commented Jan 9, 2023

flodaniel commented Apr 13, 2023

micalevisk Dec 13, 2021 •

edited

Loading

leonardovillela Dec 13, 2021 •

edited

Loading

leonardovillela Dec 13, 2021 •

edited

Loading

leonardovillela commented Dec 13, 2021 •

edited

Loading

micalevisk commented Dec 13, 2021 •

edited

Loading

leonardovillela commented Dec 19, 2021 •

edited

Loading

micalevisk commented Dec 22, 2021 •

edited

Loading

leonardovillela commented Jan 8, 2022 •

edited

Loading

leonardovillela commented Aug 17, 2022 •

edited

Loading

micalevisk commented Oct 5, 2022 •

edited

Loading