ngrx / platform

Reactive State for Angular
https://ngrx.io
Other
8.04k stars 1.98k forks source link

RFC: Mock Store #915

Closed TeoTN closed 6 years ago

TeoTN commented 6 years ago

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[x] Feature request
[ ] Documentation issue or request

What is the current behavior?

There's no simple way to mock a Store for testing purposes.

Expected behavior:

When testing a component or an effect, I'd like to be able to inject a fake Store to the tested entity. It became increasingly non-trivial after the pipable operators were introduced.

For instance, one may want to test behavior component with a given state that is a result of many various actions. Similarly, one may want to test an effect with various fixtures for state. Across the web there are multiple snippets with custom implementation of MockStore that extends either Store or BehaviorSubject but all of them are outdated. The presence of such snippets suggests though that there's a real need for some testing utils. I propose introduction of @ngrx/store/testing module which would contain provideMockStore for providing MockStore in place of Store and an implementation of MockStore that provides nextMock method that replaces current state just like BehaviorSubject.next does on Subject.

Version of affected browser(s),operating system(s), npm, node and ngrx:

Newest ngrx, reasonably new npm, node, Chrome and Windows.

Other information:

I'll make a contribution with MockStore class and a provider, if you feel it's a valid issue.

SerkanSipahi commented 6 years ago

I was really exited about that we getting mock store, selector, etc but i think its not ready for usage its makes more complicated. I dont know.

Well, mocking selector and store are not worked well for me or just lets say: really hard to do that simple (maybe im stupid) or maybe it is not fully developed yet, i dont know! For me a test setup should be very easy without or with less think about it.

For me it workes very well and easy to test my effect with the real store as @alex-okrushko suggested!

effect:

  @Effect({ dispatch: false }) toggleAppElement$ = this.actions$.pipe(
    ofType<Fullscreen>(FullscreenActionTypes.Event),
    switchMap(_ => combineLatest(
      this.store.pipe(select(fromRoot.getFullscreen)),
      this.store.pipe(select(fromRoot.getAppElement), skip(1)),
      this.video.getElement$<HTMLVideoElement>('video')),
    ),
    filter(([, appEl]) => appEl instanceof Element),
    tap(([fullscreen, appEl, videoEl]) =>
      this.toggleAppElement(fullscreen, appEl, videoEl)
    ),
  );

effect.spec:

describe('FullscreenEffects', () => {

  let store$: Store<fromRoot.State>;
  let fullscreenEffects: FullscreenEffects;
  let videoService: VideoService;
  let fakeEvent = (): Event => new Event('fakeName');
  let fakeElement = (): Element => document.createElement('div');

  beforeEach(() => {
    TestBed.configureTestingModule({
      imports: [
        StoreModule.forRoot(reducers, { metaReducers }),
      ],
      providers: [
        Store,
        FullscreenEffects,
        Actions,
        VideoService,
      ],
    });

    fullscreenEffects = TestBed.get(FullscreenEffects);
    videoService = TestBed.get(VideoService);
    store$ = TestBed.get(Store);
  });

  it('should call fullscreenEffects.toggleAppElement', done => {
    spyOn(videoService, 'getElement$').and.returnValue(of(fakeElement()));
    spyOn(fullscreenEffects, 'toggleAppElement');

    fullscreenEffects.toggleAppElement$.subscribe(_ => {
      expect(fullscreenEffects.toggleAppElement).toHaveBeenCalledTimes(1);
      done();
    });

    store$.dispatch(new AppElement(fakeElement()));
    store$.dispatch(new Fullscreen(fakeEvent()));
  });

});

Now im not agree with @TeoTN of: It's probably a waste of time, programmers' keyboards and CPUs power!

what a waste of time? it was really easy! cpu? i have not 1000 effects thats run in tests!

nasreddineskandrani commented 6 years ago

with @TeoTN proposal you can set a state in the store without triggering actions. In your case, you are not triggering actions to set a state. But if you had to trigger actions @TeoTN proposal is better. And then his comment would have applied t's probably a waste of time, programmers' keyboards and CPUs power about triggering actions.

SerkanSipahi commented 6 years ago

with @TeoTN proposal you can set a state in the store without triggering actions

But i have to triggering a action. How should i catch ofType<Fullscreen>(FullscreenActionTypes.Event)

In your case, you are not triggering actions to set a state

See im triggering a action(s) and setting a new state:

store$.dispatch(new Fullscreen(fakeEvent()));
store$.dispatch(new AppElement(fakeElement()));

But if you had to trigger actions @TeoTN proposal is better

It did not worked for me. I can just set a state with nextMock and thats it. How to go through ofType<Fullscreen>(FullscreenActionTypes.Event)

nasreddineskandrani commented 6 years ago

Sorry i didn't see it :)

  1. you need to trigger the effect. This is the action you can avoid store$.dispatch(new AppElement(fakeElement())); not store$.dispatch(new Fullscreen(fakeEvent())); which fire the stream. You can avoid to trigger the actions you need to make select operator grab the data you want in the combineLatest by mocking selectors. --> You can check marble testing for effects also, it helps.

  2. I already explained one case with common component where triggering actions isn't welcome here -> https://github.com/ngrx/platform/issues/915#issuecomment-405089678

i ll give you another case related to your example, lets say select(fromRoot.getAppElement) rely on a state that need 1 other action ALHPA to happen before to select proprely what you need. This action ALHPA trigger 4 backend calls. You need to go mock all these 4 api calls to get the store in the right state before triggering the effect. Mocking the selector allow you to avoid the mock of the 4 apis calls.... by copy/pasting the content of your selector grabbed at runtime (debug mode). And dont worry about updating the mocking of api calls in this test if they change overtime when the end data type in the selector remain the same. => You test only the current unit.

*this is not common for me to see 'dom' stuff in effect ( ), you can poke me in gitter in private if you wanna talk about this or ask in gitter/ngrx. I built multi app with redux react/angular never had to do that.

marcelnem commented 6 years ago

@SerkanSipahi can you post the full example how you made it work with a selector which is not wrapped? I tried it in the example and I get an error. https://stackblitz.com/edit/ngrx-component-testing-simplified-7fd5nk?file=src/app/luffy/luffy.component.spec.ts

nasreddineskandrani commented 6 years ago

@marcelnem i tried it it's not working for me without pipe i can't spend more time on this. Looking forward to see what ngrx team decide after all these interactions.

SerkanSipahi commented 6 years ago

@marcelnem @nasreddineskandrani I think here is a misunderstanding! What i meant was, its not possible to compile that thing when its not wrapped by pipe!

I add spyOn(luffySelectors, 'selectPowerB').and.returnValue(of(true)); into your beforeEach example inside of component fast testing with selectors mocking and i get no compile error like on my env: Error: <spyOn> : getFullscreen is not declared writable or has no setter

When i do that with my selectors to spyOn a selector which is not wrapped by pipe i get an error.

Which version of ngrx you are using?

SerkanSipahi commented 6 years ago

1.) ... you need to trigger the effect. This is the action you can avoid store$.dispatch(new AppElement(fakeElement())); not store$.dispatch(new Fullscreen(fakeEvent())); which fire the stream....

2.) ... i ll give you another case related to your example, lets say select(fromRoot.getAppElement) rely on a state that need 1 other ac

@nasreddineskandrani thank you for your good explanation! Now i got it. It took a while until I understood it (it was to much: mock.selector, mock.next, marble tests, etc. etc)

For just clarification(to improve my understanding): in my case it make no sense to mock the store (nextMock), right? Or is there something what i not see? maybe, imagine select(fromRoot.getAppElement) use internally other selectors to bulid the state for fromRoot.getAppElement. As far as I understand it, i have to mock then each selector, one by one but with store.nextMock i could just set the state easily.

brandonroberts commented 6 years ago

Update: I left some feedback on the PR for the proposed testing API for Store here https://github.com/ngrx/platform/pull/1027#issuecomment-421063561

Leave feedback if you have any here and not there.

nasreddineskandrani commented 6 years ago

@brandonroberts I totally agree with you testing component this way will work and it's better than trigger actions :+1: so setState solve the original main issue

some argumentations on why, in my opinion, mocking selectors could be good also!

We shouldn't focus on mocking selectors. If you want to test selectors, provide a state that the selectors can use or use StoreModule.forRoot() with reducers for integration.

From your statement, i wanted to say that in my case i don't want to test selectors when i want to unit test components. why? Because integration tests are slower and should be fewer than unit tests to fail faster.

1. Also there is a way the projector in selector built-in in ngrx to unit tests selectors ->to be consitent i saw a unit test ability in the lib for components as a nice to have. https://blog.angularindepth.com/how-i-test-my-ngrx-selectors-c50b1dc556bc

conclusion section of article:

A selector can be unit tested, you don’t need a (mocked) store in order to test your selectors.

2. By your statement you don't provide a way to unit test components (doc update or code update) and encourage integration tests with real selectors to test components. I am insisting again about the fact it's slower and it may be a problem for some.

3. i know :) i am taking it far but another little point, when it fails you know it's in the component not the selector => More isolated.

thank you for the answer and i am happy setState is going in to stop the need of triggering actions to be able to test a smart component

brandonroberts commented 6 years ago

@nasreddineskandrani I see what you are saying. If you want to unit test a component that has selectors, override the class variables with test observables. Here is a simple example of overriding the selector itself, versus setting the state and using the selector as-is.

https://stackblitz.com/edit/ngrx-test-template?file=app%2Fcounter%2Fcounter.component.spec.ts

nasreddineskandrani commented 6 years ago

thank you very much so simple 👍 now that you mention it here devs will see it maybe a little section in the doc about what you just posted. You know more than me :) if it deserve a place in the doc. I am not that bad in habit but honestly i didn't see this solution. thanks again

timdeschryver commented 6 years ago

@maxime1992 responding to your comment in the PR - https://github.com/ngrx/platform/pull/1027#issuecomment-421550271.

While your solution works, it's bound to jasmine, which is a downside in my opinion. If you want to mock the selectors, see @brandonroberts 's answer https://github.com/ngrx/platform/issues/915#issuecomment-421410873.

maxime1992 commented 6 years ago

@timdeschryver that's a fair point.

That said, I don't really like the idea of replacing directly the component's variable:

timdeschryver commented 6 years ago

Those are valid points :)

brandonroberts commented 6 years ago

@maxime1992 I don't disagree with those points, but I don't think we should use a Jasmine/Jest specific way to mock selectors. Using of is not the only way of doing it of course if you need to push multiple values.

I haven't tested this, but selectors could include an API you can access on the selector itself similar to release but it would short circuit the return value.

brandonroberts commented 6 years ago

Here is a newer version that lets you override selectors in addition to state. It adds a method to the returned selector to let you set the result value that will be returned.

import { Injectable, Inject, OnDestroy } from '@angular/core';
import {
  Store,
  ReducerManager,
  ActionsSubject,
  ActionReducer,
  Action,
  ScannedActionsSubject,
  INITIAL_STATE,
  MemoizedSelector,
  MemoizedSelectorWithProps,
} from '@ngrx/store';
import { BehaviorSubject } from 'rxjs';

@Injectable()
export class MockState<T> extends BehaviorSubject<T> {
  constructor() {
    super({} as T);
  }
}

@Injectable()
export class MockReducerManager extends BehaviorSubject<
  ActionReducer<any, any>
> {
  constructor() {
    super(() => undefined);
  }
}

@Injectable()
export class MockStore<T> extends Store<T> {
  static selectors = new Set<
    MemoizedSelector<any, any> | MemoizedSelectorWithProps<any, any, any>
  >();

  constructor(
    private state$: MockState<T>,
    actionsObserver: ActionsSubject,
    reducerManager: ReducerManager,
    public scannedActions$: ScannedActionsSubject,
    @Inject(INITIAL_STATE) private initialState: T
  ) {
    super(state$, actionsObserver, reducerManager);
    this.resetSelectors();

    this.state$.next(this.initialState);
  }

  setState(state: T): void {
    this.state$.next(state);
  }

  overrideSelector<T, Result>(
    selector: MemoizedSelector<T, Result>,
    value: Result
  ): MemoizedSelector<T, Result>;
  overrideSelector<T, Result>(
    selector: MemoizedSelectorWithProps<T, any, Result>,
    value: Result
  ): MemoizedSelectorWithProps<T, any, Result>;
  overrideSelector<T, Result>(
    selector:
      | MemoizedSelector<any, any>
      | MemoizedSelectorWithProps<any, any, any>,
    value: any
  ) {
    MockStore.selectors.add(selector);
    selector.setResult(value);
    return selector;
  }

  resetSelectors() {
    MockStore.selectors.forEach(selector => selector.setResult());
    MockStore.selectors.clear();
  }

  dispatch(action: Action) {
    super.dispatch(action);
    this.scannedActions$.next(action);
  }

  addReducer() {
    // noop
  }

  removeReducer() {
    // noop
  }
}

export function provideMockStore<T>(config: { initialState?: T } = {}) {
  return [
    { provide: INITIAL_STATE, useValue: config.initialState },
    ActionsSubject,
    ScannedActionsSubject,
    MockState,
    { provide: ReducerManager, useClass: MockReducerManager },
    {
      provide: Store,
      useClass: MockStore,
    },
  ];
}

If we take the auth-guard.service.spec.ts from the example app, the test now looks like this.

import { TestBed, inject } from '@angular/core/testing';
import { Store, MemoizedSelector } from '@ngrx/store';
import { cold } from 'jasmine-marbles';
import { AuthGuard } from '@example-app/auth/services/auth-guard.service';
import { AuthApiActions } from '@example-app/auth/actions';
import * as fromRoot from '@example-app/reducers';
import * as fromAuth from '@example-app/auth/reducers';
import { provideMockStore, MockStore } from '@ngrx/store/testing';

describe('Auth Guard', () => {
  let guard: AuthGuard;
  let store: MockStore<any>;
  let loggedIn: MemoizedSelector<fromAuth.State, boolean>;

  beforeEach(() => {
    TestBed.configureTestingModule({
      providers: [provideMockStore()],
    });

    store = TestBed.get(Store);
    guard = TestBed.get(AuthGuard);

    loggedIn = store.overrideSelector(fromAuth.getLoggedIn, false);
  });

  it('should return false if the user state is not logged in', () => {
    const expected = cold('(a|)', { a: false });

    expect(guard.canActivate()).toBeObservable(expected);
  });

  it('should return true if the user state is logged in', () => {
    loggedIn.setResult(true);

    const expected = cold('(a|)', { a: true });

    expect(guard.canActivate()).toBeObservable(expected);
  });
});
Bielik20 commented 6 years ago

@brandonroberts getting: selector.setResult is not a function. I am using Jest instead of Jasmine but I don't think that matters in here.

brandonroberts commented 6 years ago

@Bielik20 this code has not been merged into master yet. @TeoTN how do you want to proceed with this PR?

TeoTN commented 6 years ago

@brandonroberts I like some of the ideas from here and the PR. I was busy recently and didn't have time to incorporate them in the code, but I'll definitely take the time to improve that. Stay tuned ;)

nasreddineskandrani commented 6 years ago

@TeoTN I can pr in your fork if you are short in time and you need help (especially for the tests/doc update). I wrote you in your personal gitter, let me know over there.

tomson7777 commented 5 years ago

@brandonroberts I checked the 7.0.0-beta.1 version, and i do not see Store.overrideSelector method which you presented few comments above. I see only such class: export declare class MockStore<T> extends Store<T> { private state$; private initialState; scannedActions$: Observable<Action>; constructor(state$: MockState<T>, actionsObserver: ActionsSubject, reducerManager: ReducerManager, initialState: T); setState(nextState: T): void; addReducer(): void; removeReducer(): void; }

Shouldn't be overrideSelector method merged?

nasreddineskandrani commented 5 years ago

@tomasznastaly it's not in. They need time please read here brandon comment: https://github.com/ngrx/platform/pull/1027#issuecomment-433054427