-
Notifications
You must be signed in to change notification settings - Fork 2
test: new method to manually log in with an existing identity #40
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
base: main
Are you sure you want to change the base?
Changes from all commits
e9fa6a0
384dd34
fc7caa2
18b9eed
023895a
2d40a45
af6cba8
c0b341e
4cfad21
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -153,4 +153,96 @@ | |
| await iiPage.waitForEvent('close'); | ||
| expect(iiPage.isClosed()).toBe(true); | ||
| }; | ||
|
|
||
| createNewIdentity = async (params?: { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the difference between this new function and
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Its a mixture of what is already there, yes. But I feel like its a very abstract case, so having it as a new function gives clarity and helps for testing purposes. The first PR was basically what you suggest, but then the test didnt check all options, so having it "standalone" just makes it easier to go through all scenarios.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, aside from If we need different behavior, let's just add an option to the existing |
||
| selector?: string; | ||
| captcha?: boolean; | ||
| }): Promise<number> => { | ||
| const iiPagePromise = this.context.waitForEvent('page'); | ||
|
|
||
| await this.page.locator(params?.selector ?? '[data-tid=login-button]').click(); | ||
|
|
||
| const iiPage = await iiPagePromise; | ||
| await expect(iiPage).toHaveTitle('Internet Identity'); | ||
|
|
||
| await iiPage.locator('#registerButton').click(); | ||
| await iiPage.locator('[data-action=construct-identity]').click(); | ||
|
|
||
| if (params?.captcha === true) { | ||
| await iiPage.locator('input#captchaInput').fill('a', {timeout: 10000}); | ||
| await iiPage.locator('#confirmRegisterButton').click(); | ||
| } | ||
|
|
||
| const identityText = await iiPage.locator('#userNumber').textContent(); | ||
| const createdIdentity = parseInt(identityText!); | ||
| expect(createdIdentity).not.toBeNull(); | ||
| return createdIdentity; | ||
| } | ||
|
|
||
| manuallySignInWithIdentity = async ({ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the difference between manuallySignInWithIdentity and signInWithIdentity? While they do differ, the naming suggests a similar intent.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The naming should be better, agreed. Basically as mentioned in the other comment, the manuallySignIn is an extended form that checks for "all possible" ways to login. Since we need a "clean start" where no Identity is "known", we have the possibility to cycle through all available options. This is done inside of manuallySignInWithIdentity
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the feedback, I guess same as my previous comment.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here a few naming alternative:
The first doesn't sound too bad? Also, the JS Docs is missing, can you please add it as well. |
||
| selector, | ||
| identity | ||
| }: { | ||
| selector?: string; | ||
| identity: number; | ||
| }): Promise<void> => { | ||
| const iiPagePromise = this.context.waitForEvent('page'); | ||
|
|
||
| await this.page.locator(selector ?? '[data-tid=login-button]').click(); | ||
|
|
||
| const iiPage = await iiPagePromise; | ||
| await expect(iiPage).toHaveTitle('Internet Identity'); | ||
|
|
||
| const existingIdentityLoginLocator = iiPage.locator(`[data-anchor-id="${identity}"]`); | ||
| const firstRunManualLoginLocator = iiPage.locator('#loginButton'); | ||
| const moreOptionsLoginLocator = iiPage.locator('[data-role="more-options"]'); | ||
|
|
||
| const waitOptions: { state: 'visible'; timeout: number } = { state: 'visible', timeout: 10000 }; | ||
|
|
||
| type RaceResult = 'existingIdentityLogin' | 'firstRunManualLogin' | 'moreOptionsLogin' | 'none'; | ||
|
|
||
| async function getRaceResult( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: for consistency with our codebase I would use a |
||
| locator: { waitFor: (options: any) => Promise<any> }, | ||
|
Check failure on line 205 in src/page-objects/InternetIdentityPage.ts
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The linter disallow |
||
| successResult: RaceResult | ||
| ): Promise<RaceResult> { | ||
| try { | ||
| await locator.waitFor(waitOptions); | ||
| return successResult; | ||
| } catch { | ||
| return 'none'; | ||
| } | ||
| } | ||
|
|
||
| const identityPromise = getRaceResult(existingIdentityLoginLocator, 'existingIdentityLogin'); | ||
| const firstTimePromise = getRaceResult(firstRunManualLoginLocator, 'firstRunManualLogin'); | ||
| const fallbackPromise = getRaceResult(moreOptionsLoginLocator, 'moreOptionsLogin'); | ||
|
|
||
| const result: RaceResult = await Promise.race([ | ||
| identityPromise, | ||
| fallbackPromise, | ||
| firstTimePromise, | ||
| ]); | ||
|
|
||
| switch(result) { | ||
| case 'existingIdentityLogin': | ||
| await existingIdentityLoginLocator.first().click({ force: true }); | ||
| break; | ||
|
|
||
| case 'firstRunManualLogin': | ||
| case 'moreOptionsLogin': { | ||
| const locator = | ||
| result === 'firstRunManualLogin' ? firstRunManualLoginLocator : moreOptionsLoginLocator; | ||
| await locator.click({ force: true }); | ||
| await iiPage.fill('input[data-role="anchor-input"]', identity.toString()); | ||
| await iiPage.locator('[data-action="continue"]').click(); | ||
| break; | ||
| } | ||
| default: | ||
| throw new Error( | ||
| 'No locator found for Buttons: existingIdentity, firstRunManualLogin or moreOptionsLogin' | ||
| ); | ||
| } | ||
| await iiPage.waitForEvent('close'); | ||
| expect(iiPage.isClosed()).toBe(true); | ||
| }; | ||
| } | ||
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 the title of the test accurate, from the code is seems that we login, logout and then login again.
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 agree, the title can be somewhat missleading. Basically the login, logout and login are used to simulate the exact flow. There is an initial login, the "automated" if the Identity is showing in the list, or the fallback if the initial Login happened but the Identity is not showing in the list.
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.
Maybe we can review the title or add a comment about the flow then?
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.
Any suggestion on what to name it?
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.
Reading your feedback again and noticing the mention of or make me think: shouldn't it be two tests actually? Or did you meant and?