test: new method to manually log in with an existing identity #40

inc-man · 2025-04-07T07:22:06Z

For the new e2e environment of OISY Wallet, we need a way to manually sign in with an existing II. This could be done in a way such as this.

inc-man · 2025-04-08T12:21:08Z

I have corrected a few things already, but unsure about the waitFor timeout. Since we always have an "alternative" it can be set to a minimum too.

@AntonioVentilii just for notifications :)

peterpeterparker · 2025-04-08T12:42:52Z

src/page-objects/InternetIdentityPage.ts

    expect(iiPage.isClosed()).toBe(true);
  };
+
+  createNewIdentity = async (params?: {


What's the difference between this new function and signInWithNewIdentity? i.e. Why introduce a new function instead of enhancing the existing one, since they seem to share quite a bit of code?

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.

~~Thanks for the feedback. I'm not convinced it should be two distinct functions, but I can live with it.~~

~~However, in that case, we should improve the naming, as it wasn’t clear to me at first glance which function does what.~~

Please also add a TODO (and create a follow-up PR once this one is merged) for some post-merge cleanup/refactoring, as the new function duplicates code. This will be especially useful since I’d not like to maintain various chunks of code that reflect the exact same II flow.

Actually, aside from iiPage.locator('#displayUserContinue'), this new createNewIdentity function is almost identical to the existing signInWithNewIdentity. So I don't think we need to add it.

If we need different behavior, let's just add an option to the existing signInWithNewIdentity instead.

peterpeterparker · 2025-04-08T12:44:03Z

src/page-objects/InternetIdentityPage.ts

+    return createdIdentity;
+  }
+
+  manuallySignInWithIdentity = async ({


What's the difference between manuallySignInWithIdentity and signInWithIdentity? While they do differ, the naming suggests a similar intent.

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

Thanks for the feedback, I guess same as my previous comment.

Here a few naming alternative:

enterIdentityAndSignIn

signInWithPromptedIdentity

inputIdentityAndSignIn

The first doesn't sound too bad?

Also, the JS Docs is missing, can you please add it as well.

src/page-objects/InternetIdentityPage.ts

peterpeterparker · 2025-04-08T12:46:37Z

src/page-objects/InternetIdentityPage.ts

+      await iiPage.fill('input[data-role="anchor-input"]', identity.toString());
+      await iiPage.locator('[data-action="continue"]').click();
+    } else {
+      throw new Error('No locator found for identity, first time, or fallback');


Can we improve the error message? I guess we can parse the effective locator that was used and I'm unsure what's "fallback".

We can, I just wrote something so we have a certain error. Basically we have "Identity" which is the locator for the Identity button when login in again. "first time" is the "initial login button that only shows when no Identity is known" and "fallback" is the "Other Options" button to sign in by manually typing in the Identity.

For context: the point is, it’s not just "we". This library can also be used by the community. That’s why having a clear API and comprehensive error messages matters.

That's why I think we should introduce the locator value and improve the error msg.

I changed the naming of the types and also the error message, maybe have a look and if not satisfied I can display the exact locator value

Actually, displaying the actual locator value was the most important part of my feedback 😉. If a developer’s test fails in CI, a stack trace like 'No locator found...' would require them to dig through their code. On the other hand, showing something like 'No locator [testId="identity-login"] found...' is more user-friendly and handy.

peterpeterparker · 2025-04-08T12:47:25Z

e2e/login.spec.ts

+    createdIdentity = await iiPage.createNewIdentity();
+  });
+
+  testWithII('sign in using identity from list or fallback manually', async ({page, iiPage}) => {


Is the title of the test accurate, from the code is seems that we login, logout and then login again.

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.

Maybe we can review the title or add a comment about the flow then?

Any suggestion on what to name it?

There is an initial login, the "automated" if the Identity is showing in the list, or the fallback

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?

peterpeterparker · 2025-04-11T13:05:30Z

e2e/login.spec.ts

+    createdIdentity = await iiPage.createNewIdentity();
+  });
+
+  testWithII('sign in using identity from list or fallback manually', async ({page, iiPage}) => {


There is an initial login, the "automated" if the Identity is showing in the list, or the fallback

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?

peterpeterparker · 2025-04-11T13:08:20Z

src/page-objects/InternetIdentityPage.ts

+
+    type RaceResult = 'existingIdentityLogin' | 'firstRunManualLogin' | 'moreOptionsLogin' | 'none';
+
+    async function getRaceResult(


nitpick: for consistency with our codebase I would use a const getRaceResult = () ...

peterpeterparker · 2025-04-11T13:08:43Z

src/page-objects/InternetIdentityPage.ts

+    type RaceResult = 'existingIdentityLogin' | 'firstRunManualLogin' | 'moreOptionsLogin' | 'none';
+
+    async function getRaceResult(
+      locator: { waitFor: (options: any) => Promise<any> },


The linter disallow any

new method to manually log in with an existing identity

e9fa6a0

inc-man requested a review from peterpeterparker April 7, 2025 07:22

inc-man added 2 commits April 7, 2025 11:43

revert previous changes and extend existing signInWithIdentity

384dd34

lint

fc7caa2

inc-man marked this pull request as ready for review April 7, 2025 09:55

inc-man requested a review from a team as a code owner April 7, 2025 09:55

inc-man and others added 4 commits April 8, 2025 12:58

added new test

18b9eed

lint

023895a

Merge branch 'main' into test/manually-sign-in

2d40a45

lint

af6cba8

peterpeterparker reviewed Apr 8, 2025

View reviewed changes

changes for clarity

c0b341e

peterpeterparker reviewed Apr 11, 2025

View reviewed changes

Merge branch 'main' into test/manually-sign-in

4cfad21


		type RaceResult = 'existingIdentityLogin' \| 'firstRunManualLogin' \| 'moreOptionsLogin' \| 'none';

		async function getRaceResult(

test: new method to manually log in with an existing identity #40

Are you sure you want to change the base?

test: new method to manually log in with an existing identity #40

Uh oh!

Conversation

inc-man commented Apr 7, 2025

Uh oh!

inc-man commented Apr 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterpeterparker Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

peterpeterparker Apr 9, 2025 •

edited

Loading