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

GH-47 Add acceptance test for WebSocket scenarios #68

Merged
merged 7 commits into from Jun 13, 2019

Conversation

@ceilfors
Copy link
Member

ceilfors commented Jun 6, 2019

Attempts to close GH-47

ceilfors added 6 commits Jun 2, 2019
Implemented connect-websocket, disconnect-websocket, and update-user lambda
Error message is currently silently swallowed
@ceilfors ceilfors requested a review from Strernd Jun 6, 2019
@ceilfors

This comment has been minimized.

Copy link
Member Author

ceilfors commented Jun 6, 2019

On the adapter implementation, do you think we should actually inject body instead of ApiGatewayWebSocketEvent object?

Before

const app = async (message, { wsClient }) => {
  if (message.body.message === "order received") {
    return wsClient.send({ message: "thank you for your order" });
  }
};

After

const app = async (message, { wsClient }) => {
  if (message === "order received") {
    return wsClient.send({ message: "thank you for your order" });
  }
};
@Strernd

This comment has been minimized.

Copy link
Member

Strernd commented Jun 7, 2019

I think it should rather be

const app = async ({ message }, { wsClient }) => {
  if (message === "order received") {
    return wsClient.send({ message: "thank you for your order" });
  }
};

shouldn't it?

I would like that way, we should put the requestContext with the laconiaContext then.

@ceilfors

This comment has been minimized.

Copy link
Member Author

ceilfors commented Jun 8, 2019

Thanks @Strernd. Would you mind reviewing the rest of the acceptance test? I'm not really familiar with WebSocket, just wanted to make sure that everything looks ok and close enough to reality.

@ceilfors ceilfors merged commit 04087e8 into master Jun 13, 2019
17 checks passed
17 checks passed
License Compliance All checks passed.
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: integration_test Your tests passed on CircleCI!
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
security/snyk - package.json (ceilfors) No manifest changes detected
security/snyk - packages/laconia-acceptance-test/package.json (ceilfors) No new issues
Details
security/snyk - packages/laconia-adapter-api/package.json (ceilfors) No manifest changes detected
security/snyk - packages/laconia-adapter/package.json (ceilfors) No manifest changes detected
security/snyk - packages/laconia-batch/package.json (ceilfors) No manifest changes detected
security/snyk - packages/laconia-config/package.json (ceilfors) No manifest changes detected
security/snyk - packages/laconia-core/package.json (ceilfors) No manifest changes detected
security/snyk - packages/laconia-invoker/package.json (ceilfors) No manifest changes detected
security/snyk - packages/laconia-middleware-lambda-warmer/package.json (ceilfors) No manifest changes detected
security/snyk - packages/laconia-middleware-serverless-plugin-warmup/package.json (ceilfors) No manifest changes detected
security/snyk - packages/laconia-test-helper/package.json (ceilfors) No manifest changes detected
security/snyk - packages/laconia-test/package.json (ceilfors) No manifest changes detected
security/snyk - packages/laconia-xray/package.json (ceilfors) No manifest changes detected
@ceilfors ceilfors deleted the gh-47 branch Jun 13, 2019
@ceilfors

This comment has been minimized.

Copy link
Member Author

ceilfors commented Jun 13, 2019

Merging due to inactivity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.