build: remove koa support and migrate to jest and eslint

[rmuchall]

Hi folks,

Background
I really like this library and since maintenance has been sparse for a while I thought I'd fork it and maintain it for my own internal projects. I've made some possibly controversial changes and I expect this pull request to be rejected but I submit it in case it is of use to you.

NB: Any changes I make to my fork will be publicly available via my Github and I'll place a big disclaimer informing people they should use the original typestack package. I don't want to step on any toes - you guys are awesome and I use many of your libraries :)

With this first pull request I've been careful not to make any functional changes other than the ones listed below. So hopefully there should be no breaking changes (other than the removal of koa).

Action taken
Removed koa support
Reasoning
I don't use Koa :)
To reduce the cognitive/maintenance load
Koa has monthly downloads of just 3.4% of express downloads
https://npmcompare.com/compare/express,koa

Action taken
Replaced mocha, chai, sinon and nyc with jest
Reasoning
To normalize the test framework across all my projects
Jest appears to be a more complete framework
Jest is more popular
https://npmcompare.com/compare/jest,mocha

Action taken
Replaced chakram with axios, qs and form-data
Reasoning
chakram appears to be no longer maintained
chakram uses request under the hood which is deprecated
axios, qs and form-data appear to be popular and actively maintained

Action taken
Replaced tslint with eslint
Reasoning
tslint is deprecated and the recommended replacement is eslint

Other changes
Removed source-map-support, request and cross-env as they are no longer used
Added express typings to ExpressMiddlewareInterface
Updated all dependencies

Thanks,
Rob

[jotamorais]

Hi @rmuchall,

First of all, thanks for submitting the PR.
I agree with the normalization of the test framework, replacing chakram with Axios and also tslint with eslint, however, once I checked out your fork to test before merging, 42 out of 115 tests, failed.

I will paste the logs below for your reference:

Click to expand!

> $ npm run test                                                                                                                                                  ⬡ 12.0.0 [±rmuchall-jest-eslint ✓]

> [email protected] pretest /home/jotamorais/dev/oss/routing-controllers
> npm run lint


> [email protected] lint /home/jotamorais/dev/oss/routing-controllers
> eslint --config ./.eslintrc.js --ext .ts ./src ./test


> [email protected] test /home/jotamorais/dev/oss/routing-controllers
> rimraf coverage && jest --coverage

 PASS  test/functional/middlewares-order.spec.ts (15.862s)
 PASS  test/functional/interceptors.spec.ts (16.979s)
 PASS  test/functional/controller-methods.spec.ts
 PASS  test/functional/auth-decorator.spec.ts
 PASS  test/functional/class-transformer-options.spec.ts
 FAIL  test/functional/express-global-before-error-handling.spec.ts (20.891s)
  ● Console

    console.error node_modules/jest-jasmine2/build/jasmine/Env.js:248
      Unhandled error
    console.error node_modules/jest-jasmine2/build/jasmine/Env.js:249
      Error: listen EADDRINUSE: address already in use :::3001
          at Server.setupListenHandle [as _listen2] (net.js:1226:14)
          at listenInCluster (net.js:1274:12)
          at Server.listen (net.js:1362:7)
          at Function.listen (/home/jotamorais/dev/oss/routing-controllers/node_modules/express/lib/application.js:618:24)
          at Object.<anonymous> (/home/jotamorais/dev/oss/routing-controllers/test/functional/express-global-before-error-handling.spec.ts:58:47)
          at /home/jotamorais/dev/oss/routing-controllers/node_modules/jest-jasmine2/build/queueRunner.js:45:12
          at new Promise (<anonymous>)
          at mapper (/home/jotamorais/dev/oss/routing-controllers/node_modules/jest-jasmine2/build/queueRunner.js:28:19)
          at /home/jotamorais/dev/oss/routing-controllers/node_modules/jest-jasmine2/build/queueRunner.js:75:41
          at processTicksAndRejections (internal/process/task_queues.js:88:5)

  ● custom express global before middleware error handling › should call global error handler middleware with CustomError
.............

  ● express middlewares › should handle errors in custom middlewares

    Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.Error: Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout.

      at mapper (node_modules/jest-jasmine2/build/queueRunner.js:27:45)

  ● express middlewares › should handle errors in custom middlewares

    TypeError: Cannot read property 'status' of undefined

      191 |         return axios.get("/customMiddlewareWichThrows")
      192 |             .catch((error: AxiosError) => {
    > 193 |                 expect(error.response.status).toEqual(HttpStatusCodes.NOT_ACCEPTABLE);
          |                                       ^
      194 |             });
      195 |     });
      196 | });

      at test/functional/express-middlewares.spec.ts:193:39

  ● express middlewares › should handle errors in custom middlewares

    expect.assertions(1)

    Expected one assertion to be called but received zero assertion calls.

      188 | 
      189 |     it("should handle errors in custom middlewares", () => {
    > 190 |         expect.assertions(1);
          |                ^
      191 |         return axios.get("/customMiddlewareWichThrows")
      192 |             .catch((error: AxiosError) => {
      193 |                 expect(error.response.status).toEqual(HttpStatusCodes.NOT_ACCEPTABLE);

      at Object.<anonymous> (test/functional/express-middlewares.spec.ts:190:16)


Test Suites: 8 failed, 14 passed, 22 total
Tests:       42 failed, 115 passed, 157 total
Snapshots:   0 total
Time:        32.363s
Ran all test suites.
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! routing-controllers@0.9.0 test: `rimraf coverage && jest --coverage`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the routing-controllers@0.9.0 test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/jotamorais/.npm/_logs/2020-04-22T17_19_19_717Z-debug.log

Tests:       42 failed, 115 passed, 157 total
Snapshots:   0 total
Time:        32.363s
Ran all test suites.
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! routing-controllers@0.9.0 test: `rimraf coverage && jest --coverage`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the routing-controllers@0.9.0 test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/jotamorais/.npm/_logs/2020-04-22T17_19_19_717Z-debug.log

After trying different approaches, I believe the best fix is to use the --runInBand flag when starting Jest so that tests run sequentially rather than in parallel. You could change the npm test script in the package.json to:

"test": "rimraf coverage && jest --coverage --runInBand --detectOpenHandles"

Let me your thoughts.

[rmuchall]

Hi @jotamorais,

Many thanks for your reply and your comments :)

I do not encounter the same errors that you are experiencing but I would like to try and solve this mystery. I wonder if the issue might be environmental. May I ask:
Which version of node are you running against?
I am using v12.16.1 LTS under Windows 10 (64 bit).

Thanks,
Rob

[jotamorais]

Hi @jotamorais,

Many thanks for your reply and your comments :)

I do not encounter the same errors that you are experiencing but I would like to try and solve this mystery. I wonder if the issue might be environmental. May I ask:
Which version of node are you running against?
I am using v12.16.1 LTS under Windows 10 (64 bit).

Thanks,
Rob

@rmuchall It could be an environment/platform issue, but since this is supposed to be cross-platform, I think we should fix it for all.
My current build environment is:

node: v12.0.0

npm: 6.9.0

OS: Ubuntu 19.10 (Eoan Ermine)

[rmuchall]

@jotamorais
Yes, I understand. We must try to fix it but first we need to discover where the problem is :)
I will try changing my node version to match yours and see if I can reproduce the errors.

[rmuchall]

@jotamorais
I have tested with both my version node-v12.16.1-LTS and your version node-v12.0.0 and all tests pass without issue. I think we might need a 3rd person to test for us so we can see if they can reproduce the issues :)

[rmuchall]

@jotamorais
If possible, please could you install the latest node LTS v12.16.2 and retest? :)

[rmuchall]

@jotamorais
I wonder if it might be some kind of resource exhaustion.
Perhaps you could also try to reboot and re-test?
I will try and hunt down a friend to test on their environment for us.

[rmuchall]

@jotamorais
Please disregard the above messages. I have managed to reproduce this issue. I'll try to refactor the tests and see if I can resolve the problem without resorting to --runInBand.
Thanks for your patience :)

[jotamorais]

@jotamorais
Please disregard the above messages. I have managed to reproduce this issue. I'll try to refactor the tests and see if I can resolve the problem without resorting to --runInBand.
Thanks for your patience :)

Ok. Let me know your findings.

[rmuchall]

Hi @jotamorais,

I've completed my investigation of this issue :)
The Jest framework was not detecting the amount of CPU cores on my system correctly. This means that --maxWorkers=1 was being set silently. This equates to using the --runInBand option. Hence why the tests were succeeding in my environment but failing in yours.

The reason the tests are failing are because multiple threads are attempting to create express servers on the same port (3001). This leads to a deadlock. I attempted to refactor the tests to pull from a pool of ports. However, this solution is currently blocked because Jest does not support custom environments written in TypeScript. Although this feature is planned for Jest v26.0 (jestjs/jest#8751).

Your original suggestion of using --runInBand is therefore the only available solution.
I've modified the package.json npm script as per your suggestion.

Many thanks for your help and patience :)

[jotamorais]

Hi @jotamorais,

I've completed my investigation of this issue :)
The Jest framework was not detecting the amount of CPU cores on my system correctly. This means that --maxWorkers=1 was being set silently. This equates to using the --runInBand option. Hence why the tests were succeeding in my environment but failing in yours.

The reason the tests are failing are because multiple threads are attempting to create express servers on the same port (3001). This leads to a deadlock. I attempted to refactor the tests to pull from a pool of ports. However, this solution is currently blocked because Jest does not support custom environments written in TypeScript. Although this feature is planned for Jest v26.0 (facebook/jest#8751).

Your original suggestion of using --runInBand is therefore the only available solution.
I've modified the package.json npm script as per your suggestion.

Many thanks for your help and patience :)

Thank you!
I will merge it in the next branch and test with the other fixes and features that I'm pushing to it and are also breaking changes and later on we can release v0.9.0

[NoNameProvided]

Blocking this as tooling will be added from the other TypeStack repos.

[NoNameProvided]

Blocking

[NoNameProvided]

Hmm, I took the liberty of using your tests, (thanks!) but it seems they are failing for me too even with the runInBand flag. You can check in #601.

[NoNameProvided]

Superset by #601

[github-actions]

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