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

Error when executing test with jest and jest-circus as testRunner #3243

Closed
cedricduffournet opened this issue Nov 18, 2021 · 17 comments · Fixed by #3245
Closed

Error when executing test with jest and jest-circus as testRunner #3243

cedricduffournet opened this issue Nov 18, 2021 · 17 comments · Fixed by #3245
Labels
Accepting PRs bug community watch Someone from the community is working this issue/PR Project: Store

Comments

@cedricduffournet
Copy link
Contributor

cedricduffournet commented Nov 18, 2021

Minimal reproduction of the bug/regression with instructions:

I am not sure than this issue is related to ngrx, angular or jest package. My apologize if I am in the wrong place

  1. Clone this repo: github.com:cedricduffournet/test-ngrx-error.git
  2. Install dependencies
yarn install
  1. Run jest test
yarn jest

The following error occur in all tests with provideMockStore added to providers

 Cannot configure the test module when the test module has already been instantiated. Make sure you are not using `inject` before `R3TestBed.configureTestingModule`.
 beforeEach(() => {  
  TestBed.configureTestingModule({
                ^

If i set jest-jasmine2 as testRunner in jest config, I don't have any error.
I did not have this issue in version 12 of angular and ngrx

Expected behavior:

Test should pass without error

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s):

Angular 13, ngrx 13.0.0

@brandonroberts
Copy link
Member

This is not an NgRx issue, as we don't control testing configuration. From the GitHub repo, I don't see where your Jest config is setup properly. Check the examples here https://github.com/thymikee/jest-preset-angular/tree/main/examples

@cedricduffournet
Copy link
Contributor Author

ok, actually i did check the example, this error occur only when I add provideMockStore to providers in configureTestingModule so I thought it was NgRx issue. I don't think it comes from jest-preset-config. I'm going to check if it comes from jest or angular package

@cedricduffournet
Copy link
Contributor Author

I posted an issue in jest-preset-angular repo (thymikee/jest-preset-angular#1187). My config seems to be ok.

It seems like Angular 13 has changed something and that made provideMockStore no longer works with jest-circus runner

@cedricduffournet
Copy link
Contributor Author

if I disable destroyAfterEach in configureTestingModule all test pass https://github.com/cedricduffournet/test-ngrx-error/blob/master/src/app/books/containers/collection-page.component.spec.ts#L40 successfully

@cedricduffournet
Copy link
Contributor Author

@brandonroberts I think this is a NgRx package issue.

In angular V13 destroyAfterEach is activated by default (https://dev.to/this-is-angular/improving-angular-tests-by-enabling-angular-testing-module-teardown-38kh)

In mock_store.ts file, resetSelectors is executed in afterEach (https://github.com/ngrx/platform/blob/master/modules/store/testing/src/mock_store.ts#L21), but because TestBed is already cleaned up, injecting MockStore in afterEach test generate the error Cannot configure the test module when the test module has already been instantiated. Make sure you are not using inject before R3TestBed.configureTestingModule.

So the 1er test of the component pass successfully, all the following will failed

@cedricduffournet
Copy link
Contributor Author

cedricduffournet commented Nov 18, 2021

maybe we could use shouldTearDownTestingModule and write something like this ?

if (typeof afterEach === 'function') {
  afterEach(() => {
    try {
      const testBed = getTestBed() as any;
      const shouldTearDown = testBed.shouldTearDownTestingModule();
      if (!shouldTearDown) {
        const mockStore: MockStore | undefined = TestBed.inject(MockStore);
        if (mockStore) {
          mockStore.resetSelectors();
        }
      }
      // eslint-disable-next-line no-empty
    } catch { }
  });
}

@timdeschryver
Copy link
Member

Sorry, but I'm a bit confused.
How's this related to jest?
I tried the reproduction, but it's running karma (with a bad config).

@cedricduffournet
Copy link
Contributor Author

cedricduffournet commented Nov 18, 2021

Sorry I made a mistake in the reproduction, to run jest :

yarn jest

@guisperandio
Copy link

I was facing the same problem and figured out that it was related to the destroyAfterEach property. My questions here are:

  1. Does having this set truly make a performance impact?
  2. Should the provideMockStore works with this set to true?

@cedricduffournet @brandonroberts

@cedricduffournet
Copy link
Contributor Author

I think it should be fixed as destroyAfterEach is this is the default value in angular 13, and jest-circus is default testRunner in jest.

Or maybe it should be mentionned in NgRx doc that destroyAfterEach should be disabled in test using mockProviderStore.

@brandonroberts
Copy link
Member

Yes I think we should support this without explicitly disabling the cleanup. It looks like from the example above it can be detected whether to do it.

@minijus
Copy link
Contributor

minijus commented Nov 19, 2021

I can confirm this. Just migrated semi-sized (~350 projects) Nrwl monorepo to version 13 and got bunch of failing tests.
Majority of them (those that I've managed to look at) use provideMockStore. Adding teardownproperty with destroyAfterEach: false seems to solve the issue. However, it would be nice not to add overrides for Angular defaults when using NgRx.

@cedricduffournet
Copy link
Contributor Author

cedricduffournet commented Nov 19, 2021

@brandonroberts I can propose a PR if you'd like based on this #3243 (comment) ?

@brandonroberts
Copy link
Member

brandonroberts commented Nov 19, 2021

@cedricduffournet yep. We'll need a test to verify this also.

I was under the impression that this would be opt-in but I must have misread it.

Here is an article from @LayZeeDK about the performance impact of the teardown https://dev.to/this-is-angular/improving-angular-tests-by-enabling-angular-testing-module-teardown-38kh

@cedricduffournet
Copy link
Contributor Author

@brandonroberts there is no error in NgRx repo because it use jest-jamine2 as testRunner.
Maybe we can use jest-circus in example-app to check if testing shouldTearDownTestingModule fix this issue (I already did the migration from jest-jasmine2 to jest-circus in this repo https://github.com/cedricduffournet/test-ngrx-error) ?

@brandonroberts
Copy link
Member

Yea, if we're using any jasmine-specific APIs for testing the example-app, those would have be migrated but that seems reasonable to me.

@LayZeeDK
Copy link
Contributor

@cedricduffournet yep. We'll need a test to verify this also.

I was under the impression that this would be opt-in but I must have misread it.

Here is an article from @LayZeeDK about the performance impact of the teardown dev.to/this-is-angular/improving-angular-tests-by-enabling-angular-testing-module-teardown-38kh

The table towards the end of the article tries to sum up the settings and reads that Angular testing module teardown is opt-out as of Angular 13.0:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepting PRs bug community watch Someone from the community is working this issue/PR Project: Store
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants