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

exclude/keep/replace not working has expected #312

Closed
kekel87 opened this issue Mar 18, 2021 · 25 comments · Fixed by #472 or #530
Closed

exclude/keep/replace not working has expected #312

kekel87 opened this issue Mar 18, 2021 · 25 comments · Fixed by #472 or #530

Comments

@kekel87
Copy link

kekel87 commented Mar 18, 2021

Hi, it's me again 🙃 !

I have a component and a module with StoreModule (NGRX):

@NgModule({
  imports: [
    CommonModule,
    StoreModule.forFeature(myReducer.featureKey, myReducer.reducer),
    EffectsModule.forFeature([MyEffects]),
    MatIconModule,
  ],
  declarations: [MyComponent],
})
export class MyModule {}

When I use provide and provideMockStore for unit test, it works perfectly 👌

MockBuilder(MyComponent, MyModule )
  .provide(provideMockStore({ selectors: [{ selector: mySelectors.selectData, value: [] }]  }));

But when I want to do an integration test (with true StoreModule), i have got an error:

MockBuilder(MyComponent, MyModule )
  .keep(StoreModule.forRoot(reducers))
  .keep(StoreModule.forFeature(myReducer.featureKey, myReducer.reducer));

// OR (same)

MockBuilder(MyComponent, MyModule )
  .keep(StoreModule.forRoot({ [myReducer.featureKey]: myReducer.reducer }));

In fact, the original StoreModule.forFeature is called 😱 .
And I've got an obscure error (ngrx/ivy/madness/wathever error):
TypeError: Cannot destructure property 'strictActionImmutability' of 'undefined' as it is undefined.
But the error is not relevant.

If I dont use MyModule in MockBuilder (and mock all my stuff individually) , it's work :

MockBuilder(MyComponent)
  .mock(MatIconModule)
  .keep(StoreModule.forRoot(reducers))
  .keep(StoreModule.forFeature(myReducer.featureKey, myReducer.reducer));

Like this, I'm sure that StoreModule.forFeature is not called.

I also tried to use exclude before keep, I got the same error:

MockBuilder(MyComponent)
  .exclude(StoreModule)
  .keep(StoreModule.forRoot(reducers))
  .keep(StoreModule.forFeature(myReducer.featureKey, myReducer.reducer));

So I'm not sure how exclude() works.

And I also tried to use replace(StoreModule, StoreModule.forRoot()) but ModuleWithProviser is not alowed.

I think this issue is maybe related to one of my previous issue: #197

@satanTime
Copy link
Member

satanTime commented Mar 18, 2021

Hi,

this is a complicated topic. The original idea for such cases is to mock a module which imports .forRoot and .forFeature and then simply to call .keep(StoreModule).

Something like that MockBuilder(ComponentToTest, AppModule).keep(StoreModule).

Then ng-mocks will keep all imports of .forRoot and .forFeature in the provided module as it is.

Anyway I'll investigate the cases and try to fix related issues.

@satanTime
Copy link
Member

I was able to reproduce the issue. Working on a fix for it.

@satanTime
Copy link
Member

satanTime commented May 2, 2021

Could you check whether the next config works with MyModule?

  .keep(StoreRootModule)
  .keep(StoreFeatureModule)
  .keep(EffectsRootModule)
  .keep(EffectsFeatureModule)

@satanTime
Copy link
Member

Hm.... actually it works for me correctly...

  .keep(StoreModule.forRoot({}))
  .keep(StoreModule.forFeature(myReducer.featureKey, myReducer.reducer))
  .keep(EffectsModule.forRoot())
  .keep(EffectsModule.forFeature()))

doesn't show any error.

Could you try the same and let me know if it works?

@satanTime
Copy link
Member

Could you also check this PR: https://github.com/ike18t/ng-mocks/pull/472/files? Maybe I something missed in the tests.

@kekel87
Copy link
Author

kekel87 commented May 4, 2021

Sorry for the late reply.
While reading your PR again, I came across this:

+ Please pay attention that `MyModule` or its imports should import `.forRoot` and `.forFeature`,
+ otherwise we need to call [`.keep`](../../api/MockBuilder.md#keep) as usually:

The two modules I'm testing don't have a StoreModule.forRoot() and/or a EffectsModule.forFeature() because they are lazy-loaded modules. And forRoot() should only be called once in the main AppModule.

If I add StoreModule.forRoot() it works, but it goes against module splitting and lazy-loading, right?

@satanTime
Copy link
Member

Aha, for a lazy loaded module, you need the context from the module which loads the lazy loaded module.

so it should be something like

MockBuilder(Component, [LazyModule, AppModule]).keep(....)

I think, it is a good point to be clarified in the docs.

Would it work for you, or you would like to see it differently?

@kekel87
Copy link
Author

kekel87 commented May 4, 2021

Indeed, I never thought of mentioning to you that it was lazy-loading.

Besides, even if it's not a lazy-loaded module, there is always, more or less, one or more context modules (parent module and the AppModule).

Your solution is cool. But it may make the test context bigger if the AppModule is big (which is often the case).

It would be nice to be able to provide only what is missing (in my case I only need StoreModule.forRoot())

Like:

MockBuilder(Component, [StoreModule.forRoot(), ComponentModule]).keep(....)

or

MockBuilder(Component, ComponentModule).context(StoreModule.forRoot())
// or MockBuilder(Component, ComponentModule).module(StoreModule.forRoot())

@satanTime
Copy link
Member

satanTime commented May 4, 2021

Aha, got it. Then I would say the first one wouldn't work, because [StoreModule.forRoot(), ComponentModule] means both of them will be mocked.

and something like MockBuilder(Component, ComponentModule).import(StoreModule.forRoot()) might be a solution.
It is doable currently via MockBuilder(Component, ComponentModule).keep(StoreModule.forRoot()), but I'm afraid that not everybody understands that .keep or .mock add modules into imports if builder hasn't met them during build process.

So in your case it should be something like

MockBuilder(Component, LazyModule).keep(StoreModule.forRoot()).keep(StoreFeatureModule);

looks a bit ugly, but it should do the string.

@kekel87
Copy link
Author

kekel87 commented May 5, 2021

We'll have to check that it works well. Because with the .keep I get the error from the beginning:
TypeError: Cannot destructure property 'strictActionImmutability' of 'undefined' as it is undefined.

@satanTime
Copy link
Member

Is it the same as MockBuilder(Component, LazyModule).keep(StoreModule.forRoot()).keep(StoreFeatureModule);? or a different setup. It would be great If you could provide a min example on stackblitz. And then on the weekend I'll release a new version with the fix.

@satanTime
Copy link
Member

satanTime commented May 5, 2021

Aha, I was able to get Cannot destructure property 'strictActionImmutability' of 'undefined' as it is undefined..

This is because this is an optional token which hasn't been provided, but ng-mocks doesn't cover this case and providers undefined for it. and because undefined.strictActionImmutability is an error - we get it.

Adding a fix to skip optional dependencies if they are not provided.

@kekel87
Copy link
Author

kekel87 commented May 5, 2021

Well done! I can't wait to test this ! 👍

@satanTime
Copy link
Member

Here you go!
ng-mocks.zip

This is the way: MockBuilder(Component, LazyModule).keep(StoreModule.forRoot()).keep(StoreFeatureModule);
Looking forward to your feedback.

@kekel87
Copy link
Author

kekel87 commented May 6, 2021

Yeah !! It's work perfectly. Nice job !!

@satanTime
Copy link
Member

Thanks for checking! On the weekend I'm releasing it.

@Dji75
Copy link
Contributor

Dji75 commented May 6, 2021

Really happy for this fix ! :D

satanTime added a commit that referenced this issue May 6, 2021
fix(mock-builder): respecting forward-ref and optional params #312
@satanTime
Copy link
Member

v11.11.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.

@Dji75
Copy link
Contributor

Dji75 commented May 11, 2021

After upgrading to v11.11.1, I have a new error:
error properties: Object({ longStack: 'TypeError: Cannot read property 'xxx' of undefined

The undefined seems to be the root store.

I'm exactly in the same context as kekel87 (lazy loaded module):

      MockBuilder(MyComponent, LazyModule)
        .keep(FormsModule)
        .keep(ReactiveFormsModule)
        .keep(StoreModule.forRoot({}))
        .keep(StoreModule.forFeature(...))
        .keep(StoreModule.forFeature(...))

@satanTime satanTime reopened this May 11, 2021
@satanTime
Copy link
Member

Hi @Dji75,

Thanks for the info. Could you provide a min repo / example?

@satanTime
Copy link
Member

satanTime commented May 11, 2021

Hi @Dji75,

I've added one more test case: https://github.com/ike18t/ng-mocks/pull/530/files, but it works well.

I think, the issue you pointed belongs to a missed reducer in forRoot declaration.
For example, when declarations in LazyModule use selectors for root reducers.

In such a case, ng-mocks cannot guess proper state of the store. I would suggest to provide here the module which declares the root reducer, for example, if it is AppModule:

MockBuilder(MyComponent, [LazyModule, AppModule])
  .keep(FormsModule)
  .keep(ReactiveFormsModule)
  .keep(StoreRootModule)
  .keep(StoreFeatureModule)

Then all reducers will be present in the context of LazyModule and all selectors for the root state also should work well.

satanTime added a commit that referenced this issue May 11, 2021
@satanTime
Copy link
Member

v11.11.2 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.

@Dji75
Copy link
Contributor

Dji75 commented May 20, 2021

I've finally go ahead on this topic :)

Thanks for the provided code but I still have some issues.

The first one is that my AppModule contained a mecanism with metaReducers which persist the store in sessionStorage.
I was able to disable it by mocking the corresponding util
First problem fixed.

The second one was due to BrowserAnimationsModule (in AppModule) which I have to replace by NoopAnimationsModule (.keep(NoopAnimationsModule)). No, I don't have any animation in my component 🤷‍♂️
So I consider this problem workarounded (and I can deal with it).

The last one is the tricky one ;)
I have lot of different store.dispatch in my unit tests and it seems previously dispatched values remains between tests if I don't dispatch again new (or initial ones) values.

With those "reset", all tests are successful, else some of them fail (depending on seed).

Any idea what happens here ?

@satanTime
Copy link
Member

Hi @Dji75,

  • NoopAnimationsModule
    Might you provide an example of setup with NoopAnimationsModule? I meet issues time to time with it, but I've never had a properly failing environment.

  • store.dispatch
    how do you mock the store.dispatch? Might you provide an example of failing test? Also feel free to drop me a email or find me on linked it. A call with screen sharing would solve everything faster, if it is fine for you :)

@Dji75
Copy link
Contributor

Dji75 commented May 21, 2021

I finally found why my tests runs not using initial (empty) initial state, this was because the spy on my metaReducer util was not taken into account, so I got previous value from sessionStorage which failed some tests :(

Here is the declaration in AppModule:

    StoreModule.forRoot({}, { metaReducers: [ReducerUtils.storePersist], runtimeChecks: environment.storeRuntimeChecks }),

Here is how I set spy (using jasmine), prior to MockBuilder usage:

      spyOn(ReducerUtils, 'storePersist').and.callFake((v) => v);

Maybe it can help you to see util's declaration ;)

export abstract class ReducerUtils {
  static storePersist(reducer: ActionReducer<object>): ActionReducer<object> {
    ...

If I add some logs in this util, I can see them in console 👎
If I remove metaReducers declaration, it works.

Moreover, I see another problem using StoreDevtoolsModule. I also have to keep it (.keep(StoreDevtoolsModule)). Most of tests fails because reducer function is not called even after store.dispatch.

you asked me before how I spy on store, here it is:

  describe('integration', () => {
    let store: Store;
    ...
    const initTestBed = () => {
      ...
      return MockBuilder(MyComponent, [MyModule, AppModule])
        .keep(StoreRootModule)
        .keep(StoreFeatureModule)
        .keep(StoreDevtoolsModule)
        .keep(NoopAnimationsModule)
        .keep(FormsModule)
        .keep(ReactiveFormsModule);
    };
    beforeEach(initTestBed);

    beforeEach(() => {
      fixture = MockRender(MyComponent);
      store = TestBed.inject(Store);
      spyOn(store, 'dispatch').and.callThrough();
      fixture.detectChanges();
    });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants