-
Notifications
You must be signed in to change notification settings - Fork 108
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
CUMULUS-436: Add activities support to @cumulus/integration-tests #277
CUMULUS-436: Add activities support to @cumulus/integration-tests #277
Conversation
} | ||
|
||
const succeededDetails = JSON.parse(stepExecution.completeEvent[this.eventDetailsKeys.succeeded].output.toString()); | ||
return succeededDetails; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicky, but you don't need to define succeededDetails
, you could just return the value.
This changes the API for getting the lambda output from |
@@ -69,11 +69,13 @@ async function waitForCompletedExecution(executionArn) { | |||
let statusCheckCount = 0; | |||
|
|||
// While execution is running, check status on a time interval | |||
/* eslint-disable no-await-in-loop */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I usually prefer to add the eslint-disable comment on the same line that is causing the error. So, something like this:
await timeout(waitPeriodMs); // eslint-disable-line no-await-in-loop
packages/integration-tests/index.js
Outdated
@@ -123,7 +125,7 @@ async function startWorkflowExecution(workflowArn, inputFile) { | |||
async function executeWorkflow(stackName, bucketName, workflowName, inputFile) { | |||
const workflowArn = await getWorkflowArn(stackName, bucketName, workflowName); | |||
const execution = await startWorkflowExecution(workflowArn, inputFile); | |||
const executionArn = execution.executionArn; | |||
const { executionArn } = execution; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two lines could just be
const { executionArn } = await startWorkflowExecution(workflowArn, inputFile);
return eventScheduled && isStepEvent; | ||
}); | ||
|
||
if (scheduleEvent === null || scheduleEvent === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could just be if (!scheduleEvent) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a couple of small suggestions but, overall, 👍
Thanks @laurenfrederick @yjpa7145 for your reviews! Addressed comments and just waiting for CI to pass before merging. |
Summary: Adds support for testing output of activities in workflow tests
Addresses CUMULUS-436: Cumulus Integration Tests should include an ECS task
Changes
integration-tests/lambda.js
withintegration-tests/sfnStep.js
and removes many reference tolambda
sfnStep.js
includesLambdaStep
andActivityStep
classes which inherit fromSfnStep
integration-tests/index.js
Test Plan