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

feature: Testing module for @ngrx/store #1027

Merged
merged 7 commits into from Oct 25, 2018

Conversation

Projects
None yet
@TeoTN
Copy link
Contributor

TeoTN commented May 7, 2018

Addresses #915: This PR introduces a simple MockStore that allows setting state shape for testing purposes.
Alongside with MockStore provider function is declared, and both are structured into @ngrx/store/testing.

Please review changes and help me ensure that the testing module is setup properly, or feel free to commit appropriate changes.

@coveralls

This comment has been minimized.

Copy link

coveralls commented May 7, 2018

Coverage Status

Coverage decreased (-0.04%) to 88.359% when pulling c11e170 on TeoTN:feature/mock-store into 41a0d45 on ngrx:master.

@eppsilon

This comment has been minimized.

Copy link

eppsilon commented Jun 1, 2018

How could this be used to mock lazy-loaded feature state? The nextMock(nextState: T) method prevents providing state that isn't in the root/global state interface.

@TeoTN

This comment has been minimized.

Copy link
Contributor Author

TeoTN commented Jun 13, 2018

The purpose of this solution is to mock current state of the app, it doesn't care about the way the state was built. So it doesn't matter whether some part of state was built with forRoot or forFeature, it all goes to a single object. You probably should have global state interface apart from root one.

@timdeschryver timdeschryver referenced this pull request Aug 12, 2018

Merged

Update NgRx #296

@TeoTN TeoTN force-pushed the TeoTN:feature/mock-store branch 4 times, most recently from 85c6163 to 6eb8102 Aug 29, 2018

@TeoTN

This comment has been minimized.

Copy link
Contributor Author

TeoTN commented Aug 29, 2018

Hey @MikeRyanDev I've added a little bit more code and tests :) Can we work towards merging it?

@marcelnem

This comment has been minimized.

Copy link

marcelnem commented Sep 3, 2018

Additionally, to nextMock and spyOnDispatch, I think, it would be good to add an ability to mock selectors to the proposed MockStore implementation. Being able to mock selectors is mentioned quite often in the thread #915, so I think people like to use it.

@SerkanSipahi

This comment has been minimized.

Copy link
Contributor

SerkanSipahi commented Sep 4, 2018

@TeoTN Thank you so much for that :) Is there any documentation/examples(i saw the tests)/practices for that?

@brandonroberts

This comment has been minimized.

Copy link
Member

brandonroberts commented Sep 13, 2018

@TeoTN posting here because it will get lost in the issue. First, thanks for your work and patience! I've played around with this in the example app and I have some suggestions.

  • StoreModule.forRoot() should not be required in addition to this.
  • Mocking the dispatch depends on the developers test suite of choice, so a method to patch it should not be included
  • I think setState as the method name instead of nextMock is clearer.
  • 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.

Here is a proposed update to the testing module.

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

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

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

@Injectable()
export class MockStore<T> extends Store<T> {
  private state$ = new BehaviorSubject<T>(this.initialState as T);

  constructor(
    state$: StateObservable,
    actionsObserver: ActionsSubject,
    reducerManager: ReducerManager,
    public scannedActions$: ScannedActionsSubject,
    @Inject(INITIAL_STATE) private initialState: any
  ) {
    super(state$, actionsObserver, reducerManager);

    this.source = this.state$.asObservable();
  }

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

  dispatch<V extends Action = Action>(action: V) {
    super.dispatch(action);
    this.scannedActions$.next(action);
  }

  addReducer() {
    // noop
  }

  removeReducer() {
    // noop
  }
}

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

Thoughts?

@brandonroberts brandonroberts referenced this pull request Sep 13, 2018

Closed

RFC: Mock Store #915

@ef32

This comment has been minimized.

Copy link

ef32 commented Sep 13, 2018

@brandonroberts your proposal for the testing module above seems like a perfect fit, the selectors do not need to be mocked; by providing state initially, the required selectors should be able to work with the store. That said, here are some suggestions to slightly enhance your proposal.

the observable source field is now deprecated and in my opinion, i dont think it needs to be populated in this scenario.

  • StateObservable being an observable, can be substituted using MockState. No need to provide with useClass.

  • MockState can be generic, requiring the same generic Type as MockStore hence providing strong type checking.

  • Initial state can be initialised by calling the next method with the initialstate token in the MockStore constructor.

  • Test suites could require additional data to state, so setState can cater for such scenarios.

please find below slight modification:

import { Injectable, Inject } from '@angular/core';
import {
StateObservable,
Store,
ReducerManager,
ActionsSubject,
ActionReducer,
Action,
ScannedActionsSubject,
INITIAL_STATE,
} 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> {

// not needed
// private state$ = new BehaviorSubject(this.initialState as T);

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

//deprecated
// this.source = this.state$.asObservable();

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

setState(state: T): void {
const newState = {
...this.initialState,
...state
};
this.state$.next(newState);
}

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

addReducer() {
// noop
}

removeReducer() {
// noop
}
}

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

just a suggestion. Tested with my test suites and it works. Your thoughts?

@brandonroberts

This comment has been minimized.

Copy link
Member

brandonroberts commented Sep 14, 2018

@ef32 for the most part it looks good to me.

I'd rather leave it up the developer on the state they want to set beyond the initial state.
The deps isn't needed for the provider object.

@ef32

This comment has been minimized.

Copy link

ef32 commented Sep 14, 2018

@brandonroberts true. The deps in the provider object isn’t needed. My bad , forgot to take it out during tests

@maxime1992

This comment has been minimized.

Copy link
Contributor

maxime1992 commented Sep 15, 2018

Hey, joining the discussion because there is one point where I have a different opinion:

your proposal for the testing module above seems like a perfect fit, the selectors do not need to be mocked; by providing state initially, the required selectors should be able to work with the store

and I think it might be worth to have a solution for that :)

I've brought a piece code where we can manage and mock selectors really easily.
Initially, I thought like you that it would not be a great idea and that providing a state was a better idea. Except that in some cases it's definitely not handy.

Example:

Let say you have a store which contains:

  • Cities
  • Companies
  • Product categories
  • Products

Based on some filters given by the user, you want to display for a given city, company and product category, the total sum of the products they have. In that case you'll have to provide a substantial amount of code/mocked data to have the right state. Where all you really want for this integration test is a number.

@nasreddineskandrani

This comment has been minimized.

Copy link
Contributor

nasreddineskandrani commented Sep 19, 2018

nice example @maxime1992! i ll keep it in my hat

@TeoTN

This comment has been minimized.

Copy link
Contributor Author

TeoTN commented Oct 10, 2018

Hey @brandonroberts, I like your ideas, especially getting isolated from any reducers. (I don't remember now why I've done it that way, maybe I missed some knowledge). Commit is on its way. :)

@TeoTN TeoTN force-pushed the TeoTN:feature/mock-store branch from 6eb8102 to eeecf78 Oct 10, 2018

@TeoTN TeoTN force-pushed the TeoTN:feature/mock-store branch from 623b275 to 71ca1a5 Oct 10, 2018

@TeoTN

This comment has been minimized.

Copy link
Contributor Author

TeoTN commented Oct 15, 2018

I'd appreciate some help with configuring Bazel properly... :)

@@ -0,0 +1,12 @@
import { Injectable } from '@angular/core';
import { BehaviorSubject } from 'rxjs/BehaviorSubject';

This comment has been minimized.

@brandonroberts

brandonroberts Oct 16, 2018

Member

The import here should just be rxjs

@@ -0,0 +1,9 @@
import { Injectable } from '@angular/core';
import { BehaviorSubject } from 'rxjs/BehaviorSubject';

This comment has been minimized.

@brandonroberts

brandonroberts Oct 16, 2018

Member

The import here should just be rxjs

Show resolved Hide resolved modules/store/testing/index.ts

@Injectable()
export class MockStore<T> extends Store<T> {
private stateSubject = new BehaviorSubject<T>({} as T);

This comment has been minimized.

@brandonroberts

brandonroberts Oct 16, 2018

Member

Remove stateSubject as it isn't needed?

public scannedActions$: Observable<Action>;

constructor(
private state$: StateObservable,

This comment has been minimized.

@brandonroberts

brandonroberts Oct 16, 2018

Member

Type with MockState<T> instead of StateObservable and remove the additional type casting below

mockStore.dispatch(action);
});

it('should allow setting a spy on dispatch method', () => {

This comment has been minimized.

@brandonroberts

brandonroberts Oct 16, 2018

Member

Remove this test

@brandonroberts brandonroberts merged commit ab56aac into ngrx:master Oct 25, 2018

4 checks passed

ci/circleci: bazel Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
coverage/coveralls Coverage decreased (-0.04%) to 88.359%
Details
@brandonroberts

This comment has been minimized.

Copy link
Member

brandonroberts commented Oct 25, 2018

Thanks for all your work on this @TeoTN! We will revisit mocking selectors since we are going to keep store.select for now.

@brandonroberts brandonroberts added the 7.x label Oct 25, 2018

@nasreddineskandrani

This comment has been minimized.

Copy link
Contributor

nasreddineskandrani commented Oct 27, 2018

thank's for all this work
@brandonroberts the testing doc/example-app are updated with this? (i don't see it in the diff)

@TeoTN

This comment has been minimized.

Copy link
Contributor Author

TeoTN commented Oct 28, 2018

Thank you @brandonroberts for all the help provided!!

@brandonroberts

This comment has been minimized.

Copy link
Member

brandonroberts commented Oct 31, 2018

@nasreddineskandrani the testing in the example app has not been updated yet.

@nasreddineskandrani

This comment has been minimized.

Copy link
Contributor

nasreddineskandrani commented Nov 12, 2018

hi @brandonroberts,

--- [Doc] update
I saw that you moved the documentation to: https://ngrx.io/
For example if i navigate into the doc folder of effects the message:
The documentation below is for NgRx versions 6.x and older. Visit https://ngrx.io for the latest documentation.
is present.

This means the documentation can only be updated by you (the members of ngrx team) starting from v7?

--- [example-app] update
you said:

the testing in the example app has not been updated yet.

do we want to change all tests to this way?
OR
just have one isolated example of it? if so where you suggest to do it?

@brandonroberts

This comment has been minimized.

Copy link
Member

brandonroberts commented Nov 12, 2018

@nasreddineskandrani no, it does not. Anyone can still edit the docs. They are just accessed from the site now. You can still edit any of the content for the docs in the projects/ngrx.io/content folder. You can also directly edit any page from the site itself by clicking on the pencil in the top right corner of any page.

All the test documentation will be updated to use the testing module. The testing guide will be updated with better documentation on testing.

@nasreddineskandrani

This comment has been minimized.

Copy link
Contributor

nasreddineskandrani commented Nov 28, 2018

@brandonroberts i am checking some major issues in https://github.com/jest-community/vscode-jest
I'll come back to this update on example app if no one do it in 2 weeks. Can we keep track of it with a dedicated github issue? possibly doing it step by step not one big PR. What do you think?

@brandonroberts

This comment has been minimized.

Copy link
Member

brandonroberts commented Nov 28, 2018

Sure. The guide rewrites are being tracked here #1383

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment