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

WebSocket Integration Tests (Take 2) #814

Closed

Conversation

computerpunc
Copy link
Contributor

@computerpunc computerpunc commented Sep 19, 2019

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:

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

@computerpunc computerpunc changed the title WebSocket Integration Tests ( WebSocket Integration Tests (Take 2) Sep 19, 2019
@computerpunc computerpunc mentioned this pull request Sep 19, 2019
5 tasks
@dnalborczyk
Copy link
Collaborator

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

@computerpunc
Copy link
Contributor Author

computerpunc commented Oct 2, 2019

@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)

@dnalborczyk
Copy link
Collaborator

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

@dnalborczyk
Copy link
Collaborator

dnalborczyk commented Oct 25, 2019

@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?

@computerpunc
Copy link
Contributor Author

computerpunc commented Oct 25, 2019

@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 5eca9cf 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 8b54134 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
Copy link
Contributor Author

@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.

@dnalborczyk
Copy link
Collaborator

@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
Copy link
Collaborator

@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.

@computerpunc
Copy link
Contributor Author

@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?

@dherault
Copy link
Owner

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

@dherault dherault closed this Apr 13, 2022
@dnalborczyk dnalborczyk reopened this Nov 2, 2022
@dnalborczyk
Copy link
Collaborator

re-opening as a reminder to have a look.

@dnalborczyk dnalborczyk self-requested a review December 13, 2022 00:53
@dnalborczyk dnalborczyk self-assigned this Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants