feat(gen2-migration): add shared test utils file for test apps#14589
feat(gen2-migration): add shared test utils file for test apps#14589sanjanaravikumar-az wants to merge 7 commits intogen2-migrationfrom
Conversation
sanjanaravikumar-az
changed the title
iankhou
left a comment
iankhou
left a comment
There was a problem hiding this comment.
Could you accompany this change with a change to a test-script? That will help give context on how this code is actually used.
amplify-migration-apps/shared-test-utils/test-apps-test-utils.ts
Outdated
Show resolved
Hide resolved
amplify-migration-apps/shared-test-utils/test-apps-test-utils.ts
Outdated
Show resolved
Hide resolved
amplify-migration-apps/shared-test-utils/test-apps-test-utils.ts
Outdated
Show resolved
Hide resolved
amplify-migration-apps/shared-test-utils/test-apps-test-utils.ts
Outdated
Show resolved
Hide resolved
amplify-migration-apps/shared-test-utils/test-apps-test-utils.ts
Outdated
Show resolved
Hide resolved
| export interface AmplifyConfig { | ||
| aws_user_pools_id?: string; | ||
| aws_user_pools_web_client_id?: string; | ||
| aws_cognito_region?: string; | ||
| signinIdentifier?: SigninIdentifier; | ||
| signupAttributes?: SignupAttribute[]; | ||
| } |
There was a problem hiding this comment.
Why all optional keys? We use this in both createTestUser and resolveAuthConfig, how come we don't have separate interfaces for these two functions, since the keys that each of these functions use do not intersect at all?
There was a problem hiding this comment.
The fields are all optional because the three Cognito fields match the Amplify config JSON shape where they're optional. The auth pattern fields (signinIdentifier, signupAttributes) are optional so existing callers don't have to change anything, they just get email-only by default. Splitting it up would mean callers have to break apart the same config object into multiple pieces, and since createTestUser is the only thing reading it, I don't think we'd gain much from that.
| phoneNumber: TEST_PHONE_NUMBER, | ||
| username: TEST_USERNAME, | ||
| password: TEST_PASSWORD, | ||
| }); |
There was a problem hiding this comment.
I don't feel great about this not being configurable, although I also think the test creds should be shared. Can we keep the test creds in a separate JSON file, parameterize the credential inputs, and pass them in when we call createTestUser?
There was a problem hiding this comment.
i think thats a good idea, moved the test creds into a test-credentials.json and made createTestUser take a credentials param instead of using hardcoded constants. Callers just import the JSON and pass it in
Added the project boards test script which uses this logic. |
There was a problem hiding this comment.
Why do we need this file? The signup function should just generate this on the fly and return the necessary information to the test.
There was a problem hiding this comment.
It was initially configured that way but I received a comment on it:
"I don't feel great about this not being configurable, although I also think the test creds should be shared. Can we keep the test creds in a separate JSON file, parameterize the credential inputs, and pass them in when we call createTestUser"
So we put the creds in a separate file
| * so the Amplify auth singleton has the tokens available for API/Storage calls. | ||
| * Returns the username to use for signIn. | ||
| */ | ||
| export async function provisionTestUser( |
There was a problem hiding this comment.
Lets put this function in a dedicated file called signup.ts. Its better for us (as well as AI) to have multiple small files, rather than one large one.
| }; | ||
| } | ||
|
|
||
| export class TestRunner { |
There was a problem hiding this comment.
Split all the functions and classes related to test runner to a separate file called runner.ts
There was a problem hiding this comment.
Rename the directory to _test-common. We use _ to differentiate this directory from normal apps. Also add a README.md with some basic explanation to what this directory contains.
|
|
||
| // Step 1: SignUp | ||
| try { | ||
| await cognitoClient.send(new SignUpCommand(signUpInput)); |
There was a problem hiding this comment.
Lets use AdminCreateUserCommand instead so that it also works with user pools that don't have self-sign up enabled.
3 participants
Created a shared test-utils.ts file containing shared code among the amplify migration apps. It contains the main test runner and the authentication flow using Cognito APIs. The authentication flow is made to handle all different modes of sign in including email and phone number.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.