chore(pipes-sources): update integration tests to use SqsTarget #31589
Conversation
github-actions
bot
added
p2
distinguished-contributor
aws-cdk-automation
added
the
pr/needs-community-review
|
@moelasmar Likewise here, could you review please :) |
msambol
force-pushed
the
pipes-sources-int-tests
branch
from
8236e45
to
5476688
Compare
| @@ -1,5 +1,7 @@ | |||
| import { randomUUID } from 'crypto'; | |||
| import { ITarget, InputTransformation, Pipe, TargetConfig } from '@aws-cdk/aws-pipes-alpha'; | |||
| import { InputTransformation, Pipe } from '@aws-cdk/aws-pipes-alpha'; | |||
| // eslint-disable-next-line import/no-extraneous-dependencies | |||
There was a problem hiding this comment.
I'm not sure the proper way to handle this. If I add @aws-cdk/aws-pipes-targets-alphs to devDependencies, it creates a circular dependency. is eslint-disable-next-line import/no-extraneous-dependencies OK to have here?
There was a problem hiding this comment.
@moelasmar I may need your input here. Do I need to go back to those TestClasses?
There was a problem hiding this comment.
sorry @msambol I missed checking this PR, I will take a look to it tomorrow.
There was a problem hiding this comment.
Thanks! The build is failing but those look like good imports to me in the integration test?
@aws-cdk/aws-pipes-sources-alpha: /codebuild/output/src3454251283/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-pipes-sources-alpha/test/integ.dynamodb.ts
@aws-cdk/aws-pipes-sources-alpha: 3:27 error Unable to resolve path to module '@aws-cdk/aws-pipes-targets-alpha' import/no-unresolved
@aws-cdk/aws-pipes-sources-alpha: /codebuild/output/src3454251283/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-pipes-sources-alpha/test/integ.kinesis.ts
@aws-cdk/aws-pipes-sources-alpha: 3:27 error Unable to resolve path to module '@aws-cdk/aws-pipes-targets-alpha' import/no-unresolved
@aws-cdk/aws-pipes-sources-alpha: /codebuild/output/src3454251283/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-pipes-sources-alpha/test/integ.sqs.ts
@aws-cdk/aws-pipes-sources-alpha: 4:27 error Unable to resolve path to module '@aws-cdk/aws-pipes-targets-alpha' import/no-unresolved
@aws-cdk/aws-pipes-sources-alpha: ✖ 3 problems (3 errors, 0 warnings)
There was a problem hiding this comment.
@moelasmar Wondering if I should revert some of these changes and do one test-classes.ts file that contains an SqsSource in the targets directory and an SqsTarget in the sources directory, that way there are no dependencies between the two.
There was a problem hiding this comment.
sorry @msambol for the late response. It appears I initially overlooked the splitting of this service into the main module and the source and target modules that depend on it.
I now understand why we avoided using actual resources or targets in the integ test cases. I agree with your suggestion to include the test-classes.ts file in both the source and target modules.
Could you also add a comment to clarify why we’ve taken this approach, along with a note suggesting we revisit this once we graduate these modules?"
There was a problem hiding this comment.
@moelasmar The more I look at this, I prefer to leave the TestTarget in the integration file. I would like to close this PR in favor of #31980. I will then make similar changes in pipes-targets so it is consistent.
There was a problem hiding this comment.
I was thinking that you will move the TestTarget to the test-classes.ts that already exist in the testing directory, and keep using it instead of the real SqsTarget or any other real Targets.
Also, just add a comment on it for why we choose this direction.
There was a problem hiding this comment.
@moelasmar I made the change locally, but it didn't feel right to me. I would need to put two different implementations in that file, since one has an input transformation. And the current class in test-classes.ts is used for unit tests. I think I prefer to keep all the code for individual integration tests in one file...
|
@moelasmar Any thoughts here? Would love to keep going on these Pipes PRs. |
|
This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
I'm going to start a fresh PR with cleanup to pipes sources/targets integration tests. |
|
Comments on closed issues and PRs are hard for our team to see. |
This incorporates feedback from #30756 so all integration tests are uniform.
Related to #31588.