integration test overhaul #759
Conversation
edwardfoyle
requested review from
sobolk,
Amplifiyer and
srquinn21
as code owners
🦋 Changeset detectedLatest commit: c844014 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
edwardfoyle
added
the
run-e2e
Tested it out locally, and yes it appears to work with the latest version of tsx |
packages/integration-tests/src/define_backend_template_harness.ts
Outdated
Show resolved
Hide resolved
packages/integration-tests/src/define_backend_template_harness.ts
Outdated
Show resolved
Hide resolved
packages/integration-tests/src/define_backend_template_harness.ts
Outdated
Show resolved
Hide resolved
packages/integration-tests/src/define_backend_template_harness.ts
Outdated
Show resolved
Hide resolved
| assertStableLogicalIds(templates.root, 'AWS::CloudFormation::Stack', [ | ||
| 'auth179371D7', | ||
| 'data7552DF31', | ||
| 'function1351588B', | ||
| 'storage0EC3F24A', | ||
| ]); | ||
|
|
||
| assertStableLogicalIds(templates.auth, 'AWS::Cognito::UserPool', [ | ||
| 'amplifyAuthUserPool4BA7F805', | ||
| ]); | ||
|
|
||
| assertStableLogicalIds(templates.data, 'AWS::AppSync::GraphQLApi', [ | ||
| 'amplifyDataGraphQLAPI42A6FA33', | ||
| ]); | ||
| assertStableLogicalIds(templates.data, 'AWS::CloudFormation::Stack', [ | ||
| 'amplifyDataAmplifyTableManagerNestedStackAmplifyTableManagerNestedStackResource86290833', | ||
| 'amplifyDataFunctionDirectiveStackNestedStackFunctionDirectiveStackNestedStackResource1246A302', | ||
| 'amplifyDataTodoNestedStackTodoNestedStackResource551CEA56', | ||
| ]); | ||
|
|
||
| assertStableLogicalIds(templates.storage, 'AWS::S3::Bucket', [ | ||
| // eslint-disable-next-line spellcheck/spell-checker | ||
| 'amplifyStorageamplifyStorageBucketC2F702CD', | ||
| ]); |
There was a problem hiding this comment.
Should move these assertions to test projects and have them implement common abstraction?
There was a problem hiding this comment.
Perhaps? Since there are only a few integ tests right now, this seems like a premature abstraction. I think we can wait and see if a useful abstraction emerges as this test suite grows.
There was a problem hiding this comment.
One benefit of doing this that I can see is that if we load test project from directory and expect assertions to be defined then this would force new test project authors to add some assertions.
There was a problem hiding this comment.
We aren't loading these tests from a directory though, it's just a normal import. I think at this point any class would look essentially the same as the tests, just with more steps.
There was a problem hiding this comment.
My comment was assuming we go back to scanning and loading test projects from directory.
But the last sentence made me thing that perhaps we could split these tests and embed them in test project directory. I.e. decentralize this.
Feel free to proceed without solving this discussion (as this is potential YAGNI as you said). If you think it's worth recording the discussion please cut GH issue.
There was a problem hiding this comment.
I'm going to leave this for now. I think it will organically come up if/when the integration tests start to have repeated bits that can be pulled into central components.
| @@ -26,7 +26,7 @@ const schema = a.schema({ | |||
| .returns(a.ref('EchoResponse')) | |||
| .authorization([a.allow.private()]) | |||
| .function('echo'), | |||
| }); | |||
| }) as never; // Not 100% sure why TS is complaining here. The error I'm getting is "The inferred type of 'schema' references an inaccessible 'unique symbol' type. A type annotation is necessary." | |||
There was a problem hiding this comment.
did this start to show up due to changes in tsconfig.json ?
There was a problem hiding this comment.
This PR is now running the test projects through the project tsconfig rather than the tsconfig that is generated for customer projects. I'll do a little more digging to see if there's a quick fix and cut a tech debt issue if not.
There was a problem hiding this comment.
I'll cut an issue to track this. I'm not finding any useful information on what's going on here but I have confirmed that this schema builds using tsc without issue in a standalone project. When I copy the tsconfig from the standalone project into this directory, it fails with that error message.
Issue #, if available:
Description of changes:
Problem
The current CFN template snapshot testing strategy has a very low signal to noise ratio. The diffs are often just asset hash changes or other noise that bloats PRs and reviewers (aka myself) tend to just skip over the template changes because they usually don't have anything useful in them.
Furthermore, the tests themselves are not very valuable anymore. They were implemented before we had a proper e2e suite as a sanity check against some classes of regressions. However, now we have a proper e2e suite that provides stronger assertions than simply checking CDK synth.
Solution
The "in memory" integration test suite has been overhauled to use the CDK assertions library to check for specific template conditions rather than a full snapshot test. Specifically, the assertions are checking that the expected category stacks are being created and that stateful resources in those stacks have stable logical IDs. This seems like a good low-cost sanity check to provide confidence that categories are working together. Tests around individual feature configuration is already happening at the unit test level so we don't need those kinds of assertions at this level. Full e2e tests happen separately in the e2e suite.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.