-
Notifications
You must be signed in to change notification settings - Fork 825
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: fix searchable migration test on code build #13046
Conversation
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 #13046 +/- ##
===========================================
+ Coverage 0.00% 48.45% +48.45%
===========================================
Files 1296 842 -454
Lines 149743 38038 -111705
Branches 1296 7789 +6493
===========================================
+ Hits 0 18432 +18432
+ Misses 148447 18016 -130431
- Partials 1296 1590 +294
... and 1243 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
console.error('Test profile credentials request failed'); | ||
console.error(e); |
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.
do we intend to let execution continue 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.
It's a background task. We don't await it anywhere in tests. But we want to know if it fails for some reason.
} | ||
|
||
setInterval(() => { | ||
void refreshCredentials(process.env.TEST_ACCOUNT_ROLE); |
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.
should this callback set isRotationScheduled
back to false?
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.
No. setInterval
calls refresh every 15 minutes. It's a background job.
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.
I pushed an update, I hope the code is more self-explanatory now.
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.
I'm dumb and was reading this as setTimeout
not setInterval
. makes sense now
let isRotationScheduled = false; | ||
|
||
export const tryScheduleCredentialRefresh = () => { | ||
if (!process.env.IS_AMPLIFY_CI || !process.env.TEST_ACCOUNT_ROLE || isRotationScheduled) { |
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.
isn't this condition always false in CI?
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.
No. Logs here
Please migrate your code to use AWS SDK for JavaScript (v3).
--
103 | For more information, check the migration guide at https://a.co/7PzMCcy
104 | (Use `node --trace-warnings ...` to show where the warning was created)
105 | console.log
106 | Test profile credentials refresh was scheduled
107 |
108 | at tryScheduleCredentialRefresh (../amplify-e2e-core/src/utils/credentials-rotator.ts:56:11)
109 |
110 | console.log
111 | CLI_REGION set to: us-east-2. Overwriting AWS_REGION and AWS_DEFAULT_REGION
112 |
113 | at src/setup-tests.ts:24:13
114 |
115 | (node:531) NOTE: We are formalizing our plans to enter AWS SDK for JavaScript (v2) into maintenance mode in 2023.
116 |
117 | Please migrate your code to use AWS SDK for JavaScript (v3).
118 | For more information, check the migration guide at https://a.co/7PzMCcy
119 | (Use `node --trace-warnings ...` to show where the warning was created)
120 | console.log
121 | /tmp/amplify-e2e-tests/ad91ed979b5745deb278_859814
122 |
123 | at createNewProjectDir (../amplify-e2e-core/src/index.ts:126:11)
124 |
125 | console.log
126 | Test profile credentials refreshed
127 |
128 | at refreshCredentials (../amplify-e2e-core/src/utils/credentials-rotator.ts:32:13)
129 |
130 | console.log
131 | Test profile credentials refreshed
132 |
133 | at refreshCredentials (../amplify-e2e-core/src/utils/credentials-rotator.ts:32:13)
134 |
135 | console.log
136 | Test profile credentials refreshed
137 |
138 | at refreshCredentials (../amplify-e2e-core/src/utils/credentials-rotator.ts:32:13)
139 |
140 | console.log
141 | Skipping cloud deletion since we are in CI, and cleanup script will delete this stack in cleanup step.
142 |
143 | at src/__tests__/transformer-migrations/searchable-migration.test.ts:43:15
144 |
145 | PASS src/__tests__/transformer-migrations/searchable-migration.test.ts (3469.739 s)
146 | transformer model searchable migration test
147 | ✓ migration of searchable directive - search should return expected results (3442393 ms)
Description of changes
This PR:
Out of scope:
This mechanism can be further extended to support child test account credential rotations if such need occurs.
However, today every other test manages to finish every attempt below 1hr threshold (scripts assume test role per retry, see
amplify-cli/.circleci/local_publish_helpers.sh
Line 233 in 9793be6
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.