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

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

Conversation

leonardovillela
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • 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: #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

await this.addDynamicMetadata(
token,
dynamicMetadata,
[].concat(scope, type),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@micalevisk micalevisk Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@leonardovillela leonardovillela Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 =)

Copy link
Contributor Author

@leonardovillela leonardovillela Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@joebowbeer
Copy link

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

@leonardovillela
Copy link
Contributor Author

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
Copy link
Contributor Author

leonardovillela commented Dec 13, 2021

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

@micalevisk
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

micalevisk commented Dec 13, 2021

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
Copy link

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Member

I believe you forgot to push the source codes

image

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

leonardovillela commented Dec 19, 2021

I believe you forgot to push the source codes

image

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

@kamilmysliwiec
Copy link
Member

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

@leonardovillela
Copy link
Contributor Author

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
Copy link
Member

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

@micalevisk
Copy link
Member

micalevisk commented Dec 22, 2021

@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

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

and then replace it on middlewareCollection Set

@leonardovillela
Copy link
Contributor Author

@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

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
Copy link
Member

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
Copy link
Contributor Author

leonardovillela commented Jan 8, 2022

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

@klutzer
Copy link

klutzer commented Aug 16, 2022

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

@leonardovillela
Copy link
Contributor Author

leonardovillela commented Aug 17, 2022

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

@JAZZ-FROM-HELL
Copy link

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

@micalevisk
Copy link
Member

micalevisk commented Oct 5, 2022

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
Copy link

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

@flodaniel
Copy link

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

@kamilmysliwiec kamilmysliwiec changed the base branch from master to 10.0.0 April 17, 2023 12:05
@kamilmysliwiec kamilmysliwiec added this to the 10.0.0 milestone Apr 17, 2023
@kamilmysliwiec kamilmysliwiec merged commit fdfabf9 into nestjs:10.0.0 Apr 17, 2023
schiemon added a commit to schiemon/nest that referenced this pull request Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants