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

feat: templategen command e2e integration tests #13984

Conversation

Sanayshah2
Copy link
Contributor

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 Sanayshah2 marked this pull request as ready for review October 21, 2024 18:07
@Sanayshah2 Sanayshah2 requested a review from a team as a code owner October 21, 2024 18:07
fs.writeFileSync(backendFilePath, updatedBackendFileContent);
}

function setEnvVariable(name: string, value: string) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete these envVars after test run

await executeCommand(commands[0], cwd);
await retry(
() => assertStepCompletion(commands[1]),
(status) => status.includes('COMPLETE') && !status.includes('IN_PROGRESS'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move IN_PROGRESS to a constant.

await retry(
() => assertStepCompletion(commands[1]),
(status) => status.includes('COMPLETE') && !status.includes('IN_PROGRESS'),
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can also move the retry config object to a constant since its the same across steps.

);
}

export async function stackRefactor(projRoot: string, category: string, bucketName: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: make category param a type to constrain the possibilities: export RefactorCategory = 'auth' | 'storage'


async function executeStep3(cwd: string, commands: string[], bucketName: string) {
setEnvVariable('BUCKET_NAME', bucketName);
await executeCommand(commands[1], cwd);
Copy link
Contributor

@abhi7cr abhi7cr Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we name the commands instead of accessing by indices for easier understanding: e.g. const [uploadSourceTemplate , uploadDestinationTemplate, ...] = commands ?

Copy link
Contributor Author

@Sanayshah2 Sanayshah2 Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we follow the Black Box Testing approach, this approach is out of scope then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup!

process.env[name] = value;
}

function extractContent(readmeContent: string, startRegex: string, endRegex: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

throw new Error('README file parsing failed to get the stack refactor commands');
}

function extractCommands(readmeContent: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

return commands;
}

function getCommandsFromReadme(readmeContent: string) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the README parsing logic is difficult to follow here. Unless a code reviewer knows a logic beforehand, it is not trivial to know what command 1,2,3 are. Some options here:

  1. Black box : Parse the README and get the list of commands, execute them one by one and doesn't care what it is. the test only cares about the end state of the stacks (i.e stack in UPDATE_COMPLETE) and resources are moved/renamed to the right stacks. (Note: You can use stack state to know if update/refactor operation is completed or not, doesn't have to get refactor object or changeset)
  2. White-box: parse the README and assume knowledge on the migration logic, like command 1 : upload to s3, command 2: update stack, command 3: refactor, and so on.
const stackMigrationHandler = StackMigrationHandler.fromTemplateGenReadme(<readme path>)
stackMigrationHandler.uploadToS3()
stackMigrationHandler.updateStack()
stackMigrationHandler.refactorStacks()
......

I would suggest the black box testing for simplicity.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great idea. We can do both kinds of testing based on what we are testing. For the happy path, we can do black box testing since we only care about the end state. We already have getCommandsFromReadme, we can just execute them one by one.
For the rollback scenario, we need to extract Rollback specific commands. So that can be its own test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, will keep the logic for the getCommandsFromReadme and executing commands one by one logic as it is. Should the commands be stored in a variable or accessing them by indices should be okay for this approach?

Copy link
Contributor

@abhi7cr abhi7cr Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah in the black box mode, we don't need the names. Iterating them one by one would suffice.

@Sanayshah2 Sanayshah2 closed this by deleting the head repository Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants