serverless-offline icon indicating copy to clipboard operation
serverless-offline copied to clipboard

WebSocket Integration Tests (Take 2)

Open computerpunc opened this issue 6 years ago • 11 comments

This PR replaces #778

Currently, WebSocket has 35 tests that are not part of npm run test. This PR will make the WebSocket tests be part of the regular testing flow.

To Do:

  • [x] Remove dependancy on running npm link in order use current branch serverless-offline in testing.
  • [x] Remove dependancy on running local DynamoDB instance for testing.
  • [x] Move from __test__/manual/websocket to __test__/integration/websocket.
  • [x] Use jest instead of mocha as a testing platform.
  • [x] Run al least one test.
  • [x] Add all main tests.
  • [x] Add all RouteSelection tests.
  • [x] Add all authorizer tests.
  • [x] Remove Dynamo DB dependancy.

computerpunc avatar Sep 19 '19 13:09 computerpunc

hey @computerpunc !! thank you so much!! let me know when this is ready to be pulled in!

dnalborczyk avatar Sep 30 '19 13:09 dnalborczyk

@dnalborczyk

This PR is ready for review. All 35 tests pass on AWS. Before merging from master all unskipped tests passed locally but, for some reason, after merging with latest master they do not.

Can you have a look to understand why no WebSocket test passes?

EDIT: alpha.40 seems to work fine but things break in alpha.41. When running serverless-offline.alpha41, the following error appears:

Error --------------------------------------------------

  Error: Invalid route options ( ) "value" must be an object
      at Object.exports.apply (.../serverless-offline-websocket-tests/node_modules/@hapi/hapi/lib/config.js:19:15)
      at new module.exports.internals.Route (/.../serverless-offline-websocket-tests/node_modules/@hapi/hapi/lib/route.js:35:16)
      at internals.Server._addRoute (.../serverless-offline-websocket-tests/node_modules/@hapi/hapi/lib/server.js:485:23)
      at internals.Server.route (.../serverless-offline-websocket-tests/node_modules/@hapi/hapi/lib/server.js:478:22)
      at HttpServer.start (.../serverless-offline-websocket-tests/node_modules/serverless-offline/dist/index.bb3aea00.js:146:18)
      at ApiGatewayWebSocket.start (.../serverless-offline-websocket-tests/node_modules/serverless-offline/dist/index.bb3aea00.js:578:28)
      at ServerlessOffline.start (.../serverless-offline-websocket-tests/node_modules/serverless-offline/dist/index.js:102:39)
      at processTicksAndRejections (internal/process/task_queues.js:93:5)
      at ServerlessOffline._startWithExplicitEnd (.../serverless-offline-websocket-tests/node_modules/serverless-offline/dist/index.js:126:5)

computerpunc avatar Oct 02 '19 19:10 computerpunc

@computerpunc thank you! sure, I'll have a look.

dnalborczyk avatar Oct 07 '19 01:10 dnalborczyk

@computerpunc just pulled down the PR as-is, and tests appear to run. is this branch based on alpha 40?

Test Suites: 1 skipped, 23 passed, 23 of 24 total
Tests:       1 skipped, 237 passed, 238 total
Snapshots:   4 passed, 4 total
Time:        17.62s

update: in order to make 'em fail, I have to rebase alpha 41 (or head of master) I assume?

dnalborczyk avatar Oct 25 '19 15:10 dnalborczyk

@computerpunc just pulled down the PR as-is, and tests appear to run. is this branch based on alpha 40?

Test Suites: 1 skipped, 23 passed, 23 of 24 total
Tests:       1 skipped, 237 passed, 238 total
Snapshots:   4 passed, 4 total
Time:        17.62s

update: in order to make 'em fail, I have to rebase alpha 41 (or head of master) I assume?

@dnalborczyk

I ran the regular old (i.e., not based on this PR) WebSocket tests on 2 computers and the WebSocket tests pass with alpha.40 but fail with alpha.41. So we can assume that on my computers something broke with the changes entered after alpha.40 and before alpha.41.

This PR was based on 5eca9cfe415b5ff698595ca2ebbb89c180688a26 which was published on Sep. 16 after alpha.40 but before alpha.41.

Each time I tried afterwards to merge with master the tests failed locally so I aborted.

On 8b54134e3cd719a37760b631c83d77995a196134 you merged for the first time master into this PR.

It's interesting because this PR never passed CI as for the same reason I see locally.

If it's running without any problem on your side, how about you merge this PR (via a branch?) into master, publish alpha.42 and I'll continue the tests on my end from this point?

computerpunc avatar Oct 25 '19 17:10 computerpunc

@dnalborczyk

In any case, I found the bug:

In src/events/websocket/HttpServer.js change const routes = [...connectionsRoutes(this._webSocketClients), catchAllRoute] to const routes = [...connectionsRoutes(this._webSocketClients), catchAllRoute()]

Hope this helps and you can add the WebSocket tests soon.

computerpunc avatar Oct 27 '19 16:10 computerpunc

@dnalborczyk

In any case, I found the bug:

In src/events/websocket/HttpServer.js change const routes = [...connectionsRoutes(this._webSocketClients), catchAllRoute] to const routes = [...connectionsRoutes(this._webSocketClients), catchAllRoute()]

Hope this helps and you can add the WebSocket tests soon.

ah, nice. thank you! I'll have a look now!

dnalborczyk avatar Oct 27 '19 23:10 dnalborczyk

@computerpunc just a quick update. applying the folder name changes locally and merge your PR-branch into master. If for some reason the tests don't pass we can fix it in master.

dnalborczyk avatar Oct 30 '19 13:10 dnalborczyk

@computerpunc just a quick update. applying the folder name changes locally and merge your PR-branch into master. If for some reason the tests don't pass we can fix it in master.

@dnalborczyk Not sure I follow. Do you want me to do anything or you are going to take care of it?

computerpunc avatar Nov 02 '19 21:11 computerpunc

2 yo PR, closing sorry, please feel free to reopen.

dherault avatar Apr 13 '22 18:04 dherault

re-opening as a reminder to have a look.

dnalborczyk avatar Nov 02 '22 22:11 dnalborczyk