nestjs / nest

A progressive Node.js framework for building efficient, scalable, and enterprise-grade server-side applications with TypeScript/JavaScript πŸš€
https://nestjs.com
MIT License
67.69k stars 7.63k forks source link

E2E tests broken when registering websockets #1114

Closed Oliboy50 closed 6 years ago

Oliboy50 commented 6 years ago

TL;DR: The issue was due to the order of the import statements in the test file (@nestjs/* stuff must be imported first) 😱

I'm submitting a...


[ ] Regression 
[x] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

Following the E2E Testing documentation, it works perfectly well to test controllers.

But as soon as you register a websocket (using providers as describe in documentation), and even if no controller is registered, we end up with the following error:

TypeError: Cannot read property 'create' of null

      23 | 
      24 |      app = moduleFixture.createNestApplication();
    > 25 |      await app.init();
         |                ^
      26 |  });
      27 | 
      28 |  afterAll(async () => {

      at SocketServerProvider.createSocketServer (node_modules/@nestjs/websockets/socket-server-provider.js:28:34)
      at SocketServerProvider.scanForSocketServer (node_modules/@nestjs/websockets/socket-server-provider.js:23:20)
      at WebSocketsController.subscribeObservableServer (node_modules/@nestjs/websockets/web-sockets-controller.js:35:60)
      at WebSocketsController.hookGatewayIntoServer (node_modules/@nestjs/websockets/web-sockets-controller.js:27:14)
      at SocketModule.hookGatewayIntoServer (node_modules/@nestjs/websockets/socket-module.js:40:35)
      at components.forEach.wrapper (node_modules/@nestjs/websockets/socket-module.js:29:44)
          at Map.forEach (<anonymous>)
      at SocketModule.hookGatewaysIntoServers (node_modules/@nestjs/websockets/socket-module.js:29:20)
      at modules.forEach (node_modules/@nestjs/websockets/socket-module.js:26:62)
          at Map.forEach (<anonymous>)
      at SocketModule.register (node_modules/@nestjs/websockets/socket-module.js:26:17)
      at NestApplication.registerModules (node_modules/@nestjs/core/nest-application.js:77:31)
      at NestApplication.init (node_modules/@nestjs/core/nest-application.js:87:20)
      at Object.beforeAll (tests/e2e/something.e2e-spec.ts:25:13)

Expected behavior

E2E testing of the whole application should work as expected even with websockets registered. Moreover, we should be able to setup E2E testing for websockets just like we do for controllers.

Minimal reproduction of the problem with instructions

https://github.com/Oliboy50/reproduce-issue-nestjs-websockets-test-e2e

What is the motivation / use case for changing the behavior?

I don't want to have to "mock" my Websocket providers in e2e tests.

Environment


Nest version: 5.2.2 ~ 5.3.9


For Tooling issues:
- Node version: 10  
- Platform:  Mac and Linux

Others:

kamilmysliwiec commented 6 years ago

I cannot reproduce your issue locally. Could you share a repository in which this error occurs?

Oliboy50 commented 6 years ago

@kamilmysliwiec sorry, I forgot to add the Error message in the issue, I updated it now

the error message is => TypeError: Cannot read property 'create' of null while calling app.init()

it's weird that you cannot reproduce locally, i'll try to take time to create a repo for this

Oliboy50 commented 6 years ago

@kamilmysliwiec I tried to make a reproducing project for this issue

I got something really small which was still reproducing the issue... but then I rm -rf node_modules and update everything and the issue disappeared and I can't reproduce it again πŸ€¦β€β™‚οΈ

So I think this has something to do with installed packages... but I'm not sure at all...

And I can't manage to fix this issue in my own project 😠

I'd love to hear if someone reproduce and know how to fix

kamilmysliwiec commented 6 years ago

Maybe just update your packages into latest versions @Oliboy50

Oliboy50 commented 6 years ago

I tried yes... but it didn't work, I have the exact same packages versions for @nestjs/* packages installed in those 2 projects... I don't understand what could go wrong :'(

kamilmysliwiec commented 6 years ago

Have you tried removing node_modules? Without reproducery repository, we cannot help too much.

Oliboy50 commented 6 years ago

πŸ‘ I'll make another reproducing repository when I'll have more time

Oliboy50 commented 6 years ago

@kamilmysliwiec I finished the minimal repository which reproduce the issue => https://github.com/Oliboy50/reproduce-issue-nestjs-websockets-test-e2e

Hope you will be able to tell me what's wrong with this issue πŸ™

kamilmysliwiec commented 6 years ago

Let's try with such test:

import { INestApplication } from '@nestjs/common';
import { Test } from '@nestjs/testing';
import { AppModule } from '../../src/app.module';

describe('SomethingController (e2e)', () => {
    let app: INestApplication;

    beforeAll(async () => {
        const moduleFixture = await Test.createTestingModule({
            imports: [AppModule],
        })
            // @TODO uncomment these lines to make tests green
            // .overrideProvider(SomethingGateway)
            // .useValue('fake provider because gateway break our tests')
            .compile();

        app = moduleFixture.createNestApplication();
        await app.init();
    });

    describe(`GET`, () => {
        it(`fake test`, async done => {
            done();
        });
    });
});
kamilmysliwiec commented 6 years ago

with reorganized imports statements.

 PASS  tests/e2e/something.e2e-spec.ts
  SomethingController (e2e)
    GET
      βœ“ fake test (1ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   0 total
Time:        2.628s
Ran all test suites.

^ this is my output

kamilmysliwiec commented 6 years ago

I believe it might be a race condition. I need to investigate this issue further

Oliboy50 commented 6 years ago

omg...

this is because I defined a moduleMapper rule in jest config which maps every import starting with @ to ../../src/

... and the ts-lint rule ordered-imports was reordering this import statement before the @nestjs/* imports

IMO, this should be either:

anyway, congrats and thank you for having spotted the bug so fast πŸ‘

Oliboy50 commented 6 years ago

I let you close the issue when you think it's ok to close it

kamilmysliwiec commented 6 years ago

Lol. πŸ˜…Glad it works now! :)

lock[bot] commented 5 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.