feat: templategen command e2e integration tests #13993

Sanayshah2 · 2024-10-23T21:41:08Z

Description of changes

Added e2e integration test for templategen command which runs as a part of the yarn e2e-migration command

Description of how you validated changes

Manually checked and verified the deployed stack resources, assertion statements

Checklist

PR description included
yarn test passes
Tests are changed or added
Relevant documentation is changed or added (and PR referenced)
New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies
Pull request labels are added

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Sanayshah2 · 2024-10-23T21:44:12Z

packages/amplify-migration-e2e/src/migrationReadmeParser.ts

+  return commands;
+}
+
+export function getCommandsFromReadme(readmeContent: string) {


Referencing the comment from the previous PR: #13984 (comment)

nit: give some sample of the readme fie, like the step 1 portion, would be helpful to visualize the file format.

packages/amplify-migration-e2e/src/gen1ResourceDetailsFetcher.ts

Sanayshah2 · 2024-10-23T21:45:40Z

packages/amplify-migration-e2e/src/templategen.ts

+
+async function executeStep3(cwd: string, commands: string[], bucketName: string) {
+  toggleEnvVariable('BUCKET_NAME', 'SET', bucketName);
+  await executeCommand(commands[1], cwd);


Referencing the comment from the previous PR: #13984 (comment)

hoangnbn · 2024-10-24T16:51:41Z

packages/amplify-migration-e2e/src/templategen.ts

+  return stackRefactorId;
+}
+
+async function executeStep1(cwd: string, commands: string[]) {


what is the commands[0] and [1] in step1? To make the code self documented, assert that the commands[x] is the command you expected. i.e assert(command[x] has prefix npx amplify....).
Similarly for step 2 and step 3 pls.

These assertions are handled in the unit tests which checks if every command is generated correctly and matches the expected string.

I see, yet I still think we need to add these assertion here for below reasons:

Self documented code. Otherwise, it is difficult to review, maintain, and extend in the future.

Easy to identify bugs. Specifically, if some command changes, unit test will adapt in the same CR. However, this e2e may (or may not) fail at random places which would take time to debug.

Unit test vs e2e serve different purposes, yet may share some assertions. It is best to decouple them and avoid making assumptions. E2e should be from end-user perspective, and it, by itself, should give us confident on the UX.

Ohh, makes sense. Thanks, will do.

But I believe, every step waits for the status to be complete and if the command doesn't get parsed correctly, the test would fail at that specific step which is reflected in the stackStatus. We are waiting for that to be complete and only then move forward, so we would know for sure which specific command failed.

Also, there are definitely some changes in the steps once the other teams resolve the bugs, so every time there is a small change, the e2e tests will every time have to be changed.

Also, if I assert every step having some prefix, won't it defeat the Black Box Testing purpose?

This line asserts the exact command in unit tests -

amplify-cli/packages/amplify-migration-template-gen/src/migration-readme-generator.test.ts

Line 67 in bc4a662

expect(fs.appendFile).toHaveBeenCalledWith(

hoangnbn · 2024-10-24T16:56:03Z

packages/amplify-migration-e2e/src/templategen.ts

+
+  const { gen2ResourceIds, gen2ResourceDetails } = await getGen2ResourceDetails(projRoot, category);
+  assert.deepEqual(gen1ResourceIds, gen2ResourceIds);
+  assert.deepEqual(gen1ResourceDetails, gen2ResourceDetails);


add assertion that these gen1ResourceIds no longer exists in gen1 stacks.

The gen2ResourceIds are fetched from the Outputs after the final npx ampx sandbox which is the final step for stack refactor. I believe the resources can only be present in one stack and CFN doesn't allow it to be in two stacks. So, I think the assertion that gen1ResourceIds no longer exists in gen1 stacks gets handled there since we are making sure the ids are fetched from the outputs and not reusing gen2ResourceIds . Also, to do this assertion, I would have to make a separate call to get the nested stack name and other call for assertion.

fair point!

abhi7cr · 2024-10-25T17:41:31Z

packages/amplify-migration-e2e/src/gen2ResourceDetailsFetcher.ts

+export async function getGen2ResourceDetails(projRoot: string, category: RefactorCategory) {
+  if (category === 'auth') {
+    return await getGen2AuthResourceDetails(projRoot);
+  } else {


nit: explicitly check for storage if we ever expand to other categories.

packages/amplify-migration-e2e/src/templategen.ts

abhi7cr · 2024-10-25T17:44:35Z

packages/amplify-migration-e2e/src/templategen.ts

+}
+
+export async function stackRefactor(projRoot: string, category: RefactorCategory, bucketName: string) {
+  const { gen1ResourceIds, gen1ResourceDetails } = await getGen1ResourceDetails(projRoot, category);


nice work on this method. Its clean and readable.

sobolk · 2024-10-29T17:46:11Z

packages/amplify-migration-e2e/src/__tests__/migration_templategen_e2e.test.ts

+      projName = `test${Math.floor(Math.random() * 1000000)}`;
+      bucketName = `testbucket${Math.floor(Math.random() * 1000000)}`;


we have generateRandomShortId somewhere in amplify-e2e-core for this kind of thing.

Thanks for pointing this out! b28e101

sobolk · 2024-10-29T17:46:39Z

packages/amplify-migration-e2e/src/__tests__/migration_templategen_e2e.test.ts

+      await copyFunctionFile(projRoot, 'function', gen1FunctionName);
+      await copyGen1Schema(projRoot, projName);
+
+      // TODO: replace below line with correct package version


is this comment still relevant ?

Irrelevant and removed - b28e101

sobolk · 2024-10-29T17:46:54Z

packages/amplify-migration-e2e/src/__tests__/migration_codegen_e2e.test.ts

      // TODO: replace below line with correct package version
-      await updatePackageDependency(projRoot, '@aws-amplify/backend', '0.0.0-test-20241003180022');
+      await updatePackageDependency(projRoot, '@aws-amplify/backend');


are todo comments relevant still ?

Irrelevant and removed - b28e101

sobolk · 2024-10-29T17:51:09Z

packages/amplify-migration-e2e/src/envVariables.ts

+type EnvVariableAction = 'SET' | 'DELETE';
+
+export function toggleEnvVariable(name: string, option: EnvVariableAction, value?: string) {
+  if (option === 'SET') {
+    process.env[name] = value;
+  } else if (option === 'DELETE') {
+    delete process.env[name];
+  }
+}


TBH. this doesn't seem to be providing lot of simplification over calling process.env directly. It's just changing syntax of how we set and unset variable without much value added.

If we want to build something that takes care of scope of env vars I'd suggest to do something like this.

function runWithEnvVarialbe(name: string, value: string, callable: ()=> void) => { process.env[name] = value; try { callable(); } finally { delete process.env[name] } }

Sure, but I think even this wouldn't quite work, because there are a few steps which involve setting an env var, and then based on those steps running correctly, the stdout of those commands, gives another envvar which need to be set. So, if the case where I had an env var used for specific steps and then other steps use other vars, this would work well. But for this use case, I think simply going with setEnvVariable and deleteEnvVariable functions would be simple to read and understand.

sobolk · 2024-10-29T17:52:32Z

packages/amplify-migration-e2e/src/gen1ResourceDetailsFetcher.ts

+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  const [gen1ClientIdWeb, gen1ClientId] = gen1ClientIds;


Please don't do this.

If this will be used earlier then this should be added later in follow up PR.
Otherwise if follow up never happens we're left with debt.

Noted. I'll keep this in mind. b28e101

…mments

feat: templategen command e2e integration tests

8f2ede1

Sanayshah2 requested a review from a team as a code owner October 23, 2024 21:41

Sanayshah2 commented Oct 23, 2024

View reviewed changes

github-advanced-security bot found potential problems Oct 23, 2024

View reviewed changes

packages/amplify-migration-e2e/src/gen1ResourceDetailsFetcher.ts Fixed Show resolved Hide resolved

Sanayshah2 commented Oct 23, 2024

View reviewed changes

Sanay Yogesh Shah added 3 commits October 23, 2024 14:51

fix: remove typo comments

07949c4

fix: resolve lint errors

d8066c2

fix: resolve yarn issues

423d6a3

hoangnbn reviewed Oct 24, 2024

View reviewed changes

fix: add function comments for better readability

b9326c0

abhi7cr reviewed Oct 25, 2024

View reviewed changes

packages/amplify-migration-e2e/src/templategen.ts Show resolved Hide resolved

abhi7cr reviewed Oct 25, 2024

View reviewed changes

packages/amplify-migration-e2e/src/templategen.ts Show resolved Hide resolved

abhi7cr reviewed Oct 25, 2024

View reviewed changes

abhi7cr approved these changes Oct 25, 2024

View reviewed changes

Sanayshah2 requested a review from sobolk October 25, 2024 19:21

sobolk reviewed Oct 29, 2024

View reviewed changes

refactor: modify setting and deleting env vars, remove unnecessary co…

b28e101

…mments

sobolk approved these changes Oct 29, 2024

View reviewed changes

Sanayshah2 merged commit 184b085 into aws-amplify:migrations Oct 29, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: templategen command e2e integration tests #13993

feat: templategen command e2e integration tests #13993

Sanayshah2 commented Oct 23, 2024

Sanayshah2 Oct 23, 2024

hoangnbn Oct 24, 2024

Sanayshah2 Oct 23, 2024 •

edited

Loading

hoangnbn Oct 24, 2024

Sanayshah2 Oct 24, 2024

hoangnbn Oct 24, 2024

Sanayshah2 Oct 24, 2024 •

edited

Loading

Sanayshah2 Oct 24, 2024

hoangnbn Oct 24, 2024

Sanayshah2 Oct 24, 2024

hoangnbn Oct 24, 2024

abhi7cr Oct 25, 2024

abhi7cr Oct 25, 2024

sobolk Oct 29, 2024

Sanayshah2 Oct 29, 2024

sobolk Oct 29, 2024

Sanayshah2 Oct 29, 2024

sobolk Oct 29, 2024

Sanayshah2 Oct 29, 2024

sobolk Oct 29, 2024

Sanayshah2 Oct 29, 2024

sobolk Oct 29, 2024

Sanayshah2 Oct 29, 2024

		projName = `test${Math.floor(Math.random() * 1000000)}`;
		bucketName = `testbucket${Math.floor(Math.random() * 1000000)}`;

		// eslint-disable-next-line @typescript-eslint/no-unused-vars
		const [gen1ClientIdWeb, gen1ClientId] = gen1ClientIds;

feat: templategen command e2e integration tests #13993

feat: templategen command e2e integration tests #13993

Conversation

Sanayshah2 commented Oct 23, 2024

Description of changes

Description of how you validated changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sanayshah2 Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sanayshah2 Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sanayshah2 Oct 23, 2024 •

edited

Loading

Sanayshah2 Oct 24, 2024 •

edited

Loading