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

test(lambda): fix sync integration test #5948

Merged
merged 9 commits into from
Nov 11, 2024

Conversation

vicheey
Copy link
Contributor

@vicheey vicheey commented Nov 7, 2024

Problem

The integration tests for sync is consistently failing. There seems to be some issue with mocking.
Since these tests are mocking the process run, we are moving these test to unit test to be executed for every CI

Solution

  • Add more test to sync unit test in sync.tes.ts
  • Remove sync integration test case from sam.test.ts in favor of added unit test in sync.tes.ts
  • Move runInTerminal() and ProcessTerminal class to a separate file(more decomposition forsync.ts` will follow)
  • Add unit test for DeployTypeWizard.test.ts
  • Add assertion methods to PrompterTester class.

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@vicheey
Copy link
Contributor Author

vicheey commented Nov 7, 2024

/runIntegrationTests

@vicheey vicheey marked this pull request as ready for review November 7, 2024 19:12
@vicheey vicheey requested a review from a team as a code owner November 7, 2024 19:12
Copy link
Member

@roger-zhangg roger-zhangg left a comment

Choose a reason for hiding this comment

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

This should be marked as a refactor PR. The scope is much larger than test.

@justinmk3
Copy link
Contributor

(more decomposition forsync.ts will follow)

Please do not overdo the decomposition. Or look for other, existing modules where parts of sync.ts might better fit. Do not add lots of granular single-purpose modules.

@vicheey
Copy link
Contributor Author

vicheey commented Nov 8, 2024

Please do not overdo the decomposition. Or look for other, existing modules where parts of sync.ts might better fit.

The decomposition will be moving create prompter methods used in Sync, Deploy and Build command into its separate file. The partial effort on this is already in here : vicheey/aws-toolkit-vscode at refactor-sam-sync-deploy-build

Do not add lots of granular single-purpose modules.

Only shared components are extracted out. Despite so, everything will remain within core/shared/sam folder specific to sam operation.

@vicheey vicheey force-pushed the fix-sync-intg-test branch from 8df7799 to 4bf1b1d Compare November 9, 2024 00:18
Copy link
Contributor

@justinmk3 justinmk3 left a comment

Choose a reason for hiding this comment

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

Thanks for improving prompterTester too!

@justinmk3
Copy link
Contributor

/runintegrationtests

Copy link
Member

@roger-zhangg roger-zhangg left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for addressing the comments, it looks much better now.

@justinmk3 justinmk3 merged commit 3ca78ba into aws:master Nov 11, 2024
23 of 25 checks passed
@vicheey vicheey deleted the fix-sync-intg-test branch November 13, 2024 22:18
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