Bug: A module that is explicitly marked to be kept should be kept even if it's imported in another module that is mocked

[DmitryEfimenko]

Description of the bug

Let's say that my component under test uses some other components that come from 3rd party library. These components are made available through a module that the 3rd party library also provides. Under the hood, the 3rd party library module is using ReactiveFormsModule.

Now, lets say that my component also does some forms stuff, so I need to use ReactiveFormsModule as well. Therefore I'd want to mock all the components that are coming from the 3rd party library, but keep ReactiveFormsModule.
I'd think that I should achieve this behavior via the following:

const deps = MockBuilder()
      .mock(AwesomeLibraryModule) // the 3rd party library module
      .keep(ReactiveFormsModule)
      .build();

This however does not seem to work. The ReactiveFormsModule is not found in the deps.imports. Please see the repro link

A repo with an example of the bug

Link: https://codesandbox.io/s/nostalgic-moore-7sezz5?file=/src/test.spec.ts

Expected vs actual behavior

The ReactiveFormsModule should be included in the deps.imports.

[satanTime]

Hi @DmitryEfimenko, it works exactly as you expect, even if AwesomeLibraryModule has been mocked, its import / export of ReactiveFormsModule will stay as it is, so you can use all directives including FormControl etc.

However , it might be present under a rebuilt module which equal ReactiveFormsModule, but isn't the same class.

Could you describe your real use-case where you are facing an issue?

[DmitryEfimenko]

hey, thanks for getting back to me so quickly, as usual! Have you looked at the repro link that I posted? I'm going to create another repro trying to show a real Angular unit test use-case where I stumbled on this issue. I'll post a link to that shortly

[satanTime]

This is how it works: https://codesandbox.io/s/red-cdn-vhbeyp?file=/src/tests/ng-mocks-change/reactive-forms.spec.ts

Because ReactiveFormsModule is kept, ControlValueAccessor isn't a mock and binds a control with an input and updates values on its change.

[DmitryEfimenko]

Thanks for the example!
I'm beginning to think that I had an issue with my unit test due to #2646. I'll close this issue and double-check my code once #2646 is resolved. Thanks!

[satanTime]

Ah, that makes sense.

For now, you can try to rely on modules instead of explicitly mocking / keeping things.

beforeEach(() => MockBuilder(ComponentToTest, ItsModuleWIthDependenciesToMock));

it should respect the place of definition. And I'll add a fix that such Components from Services are present in both places: declarations and providers.

[DmitryEfimenko]

That would be awesome! Thanks for the advice

[DmitryEfimenko]

just to have latest info in this issue, here's the latest and the minimum reproduction of the error that I could come up with:
https://codesandbox.io/s/vibrant-tree-6ghgh8?file=/src/test.spec.ts

You've asked me to see if I can help with the reproduction "on your side". Could you please clarify what you mean by that?

[satanTime]

I meant to provide a repo with the issue, so I could work on it on my side to find and fix the bug. So, basically, exactly what you did :)

Thanks for the link. I'll take a look next days where the problem is.

[satanTime]

So, .keep(NgxErrorsModule) works .mock(NgxErrorsModule) breaks.

Thanks again. That should be enough for debugging.

[satanTime]

And removal of .keep(FormsModule) also helps. Could you explain, why you need to keep both form modules instead of only reactive one?

Anyway, I should work properly with both definitions, it will be fixed.

[satanTime]

Hi @DmitryEfimenko, I also wanted to ask you about https://twitter.com/dmitryaefimenko/status/1529985415247081473, could you create a feature request and put there your thoughts about which syntax you would like to see or the problem to solve?

[DmitryEfimenko]

Could you explain, why you need to keep both form modules instead of only reactive one?

Sure thing.

TLDR:
My component happens to use both. To drive the bulk of the form, the template-driven forms are used, hence the FormsModule. However, there's a small piece in the template that relies on formControl directive, which comes from ReactiveFormsModule.

Details:
The view model value is bound to the <app-ms-to-date-input /> component. That component implements ControlValueAccessor and has a public property control: FormControl. The value of that control is set to new Date(incomingValue). So that when viewModel value that's in milliseconds is updated, the internal control value is updated with the corresponding Date object. This comes handy because the material date range component expects the Date rather than milliseconds. So I bind that internal control property to the input's formControl property. Here's approximate template that I'm working with:

<mat-date-range-input [rangePicker]="dateRangePicker">
  <input [formControl]="startDate.control" matStartDate />
  <input [formControl]="endDate.control" matEndDate />

  <app-ms-to-date-input
    #startDate
    [(ngModel)]="vm.startDate"
    name="startDate"
  ></app-ms-to-date-input>

  <app-ms-to-date-input
    #endDate
    [(ngModel)]="vm.endDate"
    name="endDate"
  ></app-ms-to-date-input>
</mat-date-range-input>

<mat-date-range-picker #dateRangePicker></mat-date-range-picker>

You see, the ngModel here comes from FormsModule and the formControl comes from ReactiveFormsModule

As for the tweet, I'm on it.

[satanTime]

So, I've taken a look deeper.

This behavior is expected. The problem here is that ReactiveFormsModule is a part of NgxErrorsModule and it's not exported.
That's why MockBuilder simply keeps it in imports of the mock NgxErrorsModule.

To make it work the module should be exported:

  beforeEach(() => MockBuilder(ComponentUnderTest)
      .mock(NgxErrorsModule)
      .keep(FormsModule, {export: true})
      .keep(ReactiveFormsModule, {export: true}));

or a shorter version

  beforeEach(() => MockBuilder(
    [ComponentUnderTest, FormsModule, ReactiveFormsModule], // things to keep and export
    NgxErrorsModule, // things to mock
  ));

I need to think if it's worth to export kept things on mock declarations by default. There is a partially related FR already: #725.
And I'll updated docs to show that all kept things should have the export flag.

[satanTime]

Just a note.

The initial expectation is that MockBuilder would be used with a related module which declares the component / directive / pipe. In this case, the module have an import of ReactiveFormsModule on the level, where the component / directive / pipe would get proper access to its exports:

beforeEach(() => MockBuilder(ComponentUnderTest, TheModuleOfComponentUnderTest)
  .keep(FormsModule)
  .keep(ReactiveFormsModule)
);

It's generating:

@NgModule({
  imports: [FormsModule, ReactiveFormsModule, MockModule(NgxErrorsModule)],
  declarations: [ComponentUnderTest],
})
class MockOfTheModuleOfComponentUnderTest {}

whereas your setup generates:

@NgModule({
  imports: [FormsModule, MockModule(NgxErrorsModule)],
  declarations: [ComponentUnderTest],
})
class MockOfMockBuilder {}

Is there a reason why you don't want to provide modules in MockBuilder?

[satanTime]

A possible solution: if MockBuilder was called with 0-1 params, then to export chain things.

[DmitryEfimenko]

ahh, this explains a lot. I think I was not using the the signature where I'd pass modules to mock in the MockBuilder because I didn't quite understand the signature of the MockBuilder. After you mentioned provided the code sample (below) it all cleared up in my mind and I will totally use this signature going forward:

MockBuilder(
    [ComponentUnderTest, FormsModule, ReactiveFormsModule], // things to keep and export
    [NgxErrorsModule], // things to mock
  )

I think it would be helpful to others if this particular example (with the arrays) is placed in the docs.
By the way, I just tested this signature with my use-case and it indeed totally fixes everything. Thank you!

Now that that's out of the way, I have to mention that I don't quite understand why we would not apply { export: true } to the things we put in .keep(). Honestly, I was not even aware of this option, which is partially where the confusion came from. I'd be in favor of applying { export: true } by default, but perhaps there are some use-cases I'm not aware of.

[DmitryEfimenko]

Awesome! I'll verify on my end once it's published to npm. Could you please comment here once that happens so that I don't miss this exciting event?

[satanTime]

Sure. There will be a comment about the related release.

I'm waiting for primeng's support of a14, and then I can release the new version.

[satanTime]

v14.0.0 has been released and contains a fix for the issue. Feel free to reopen the issue or to submit a new one if you meet any problems.