-
Notifications
You must be signed in to change notification settings - Fork 820
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
fix(e2e): local e2e test hanging for sendYes
#13010
Conversation
# Used for cleanup script | ||
CIRCLECI_TOKEN = '<your token>' # Token used for querying CircleCI to get the build details | ||
CIRCLE_PROJECT_USERNAME='<your github username>' | ||
CIRCLE_PROJECT_REPONAME='amplify-cli' | ||
|
||
AMPLIFY_PATH= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add comment that explains what this does.
Something like # A path to Amplify CLI executable. When working locally it should be set to
bin/amplify file in the workspace, for example <insert example here> if running regular e2e test. If working locally with migration test it should point to older version of amplify. Absolute paths are recommended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 295ec90
@@ -44,10 +44,12 @@ export const amplifyPush = async (cwd: string, testingWithLatestCodebase = false | |||
const pushArgs = ['push', ...(opts?.minify ? ['--minify'] : [])]; | |||
// Test amplify push | |||
await spawn(getCLIPath(testingWithLatestCodebase), pushArgs, { cwd, stripColors: true, noOutputTimeout: pushTimeoutMS }) | |||
.wait('Are you sure you want to continue?') | |||
.wait('Are you sure you want to continue "amplify push"?') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this correct change?
If e2e tests pass then it's fine I guess.
Otherwise the wait
argument can be subset of prompt for example Are you sure you want to continue
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I accidentally commit this change..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it passed the e2e test and it's sort of useful, I'll just leave it here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all e2e tests? if pipeline is green good to keep this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all e2e tests tho 🫢
Ok...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah.
if you want to capture both variants then either
.wait('Are you sure you want to continue')
OR .wait(/<some awesome regex to capture both/)
see
wait: (expectation: string | RegExp, cb?: (data: string) => void) => ExecutionContext; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not sure if we have 2 variants)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make it simple, I just removed the change 😂 4ad7631
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## dev #13010 +/- ##
===========================================
+ Coverage 0.00% 48.38% +48.38%
===========================================
Files 1296 840 -456
Lines 149743 38069 -111674
Branches 1296 7787 +6491
===========================================
+ Hits 0 18419 +18419
+ Misses 148447 18056 -130391
- Partials 1296 1594 +298
... and 1237 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
No longer needed. |
Description of changes
Issue: Local e2e test for hanging
sendYes
whenamplify_push
Error message:
Y
was sent, but no return.Short term fix: add
.sendCarriageReturn()
Long term fix: migrate from
enquirer
(npm), which is no longer under maintained. (We implementedenquirer
in #7774 because it's lighter thaninquirer
(npm) and has some list filtering feature we wanted).Other changes:
Issue #, if available
Description of how you validated changes
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.