Skip to content
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.

Sign up for GitHub

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

Adding Selecting Chain Interaction And Flow Improvements #21

Merged
merged 9 commits into from
Mar 7, 2024
Merged

Adding Selecting Chain Interaction And Flow Improvements #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4665470
chore: changing the flow of test cases; starting by creating a new wa…
rabi-siddique Mar 6, 2024
c4bf858
feature: adding behavior in import wallet flow to select a chain when…
rabi-siddique Mar 6, 2024
a39daad
feature: adding helper methods to click elements in a reliable way
rabi-siddique Mar 6, 2024
7f1a482
chore: using helper methods inside keplr.js
rabi-siddique Mar 6, 2024
a5713d7
chore: handling edge case for grabbing token values when values are l…
rabi-siddique Mar 6, 2024
aee18bf
chore: updating selector for getting wallet address and adding test c…
rabi-siddique Mar 6, 2024
0cff640
chore: addressing PR comments
rabi-siddique Mar 6, 2024
f1c1068
chore: addressing PR comments
rabi-siddique Mar 7, 2024
85c95cb
chore: replacing Agoric local with Agoric localhost
rabi-siddique Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 58 additions & 18 deletions commands/keplr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,7 @@ const keplr = {
await playwright.keplrWindow(),
);

await playwright.waitAndClick(
onboardingElements.submitChainButton,
await playwright.keplrWindow(),
);
await module.exports.handleSelectChain();

await playwright.waitForByText(
onboardingElements.phraseAccountCreated,
Expand All @@ -170,6 +167,38 @@ const keplr = {

return true;
},
async handleSelectChain() {
const chainNameExists = await playwright.waitForAndCheckElementExistence(
onboardingElements.chainNameSelector,
);

if (chainNameExists) {
await playwright.waitAndClickByText(
onboardingElements.chainName,
playwright.keplrWindow(),
);
await playwright.waitAndClick(
onboardingElements.submitChainButton,
playwright.keplrWindow(),
);
const importButtonExists =
await playwright.waitForAndCheckElementExistence(
onboardingElements.importButtonSelector,
);

if (importButtonExists) {
await playwright.waitAndClick(
onboardingElements.importButtonSelector,
playwright.keplrWindow(),
);
}
} else {
await playwright.waitAndClick(
onboardingElements.submitChainButton,
playwright.keplrWindow(),
);
}
},
frazarshad marked this conversation as resolved.
Show resolved Hide resolved
async importWalletWithPhrase(secretWords) {
await playwright.waitAndClickByText(
onboardingElements.phraseCount24,
Expand Down Expand Up @@ -238,12 +267,20 @@ const keplr = {
return true;
},

async getWalletAddress() {
await playwright.switchToKeplrWindow();
async getWalletAddress(chainName) {
playwright.switchToKeplrWindow();
await module.exports.goToHome();
const page = playwright.keplrWindow();
const newTokensSelctorExists = await playwright.waitForAndCheckElementExistence(
homePageElements.newTokensFoundSelector,
);

if (newTokensSelctorExists) {
await module.exports.addNewTokensFound(false);
}
Comment on lines +273 to +279
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont think we need to do this internally. in most cases this will not be needed
in cases when it is needed, a user will know that tokens are not currently present and can call it themselves explicitly

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that is reliable. We should not assume that the user knows. A flow should be capable of handling all edge cases.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not an edge case, the writer of a test should know if the wallet address is present or not before writing the test.

There can be cases where user does not want to press the 'add new token' button until a later test. and since our code is forcing him to add new tokens, it might ruin their flow.

Plus all our interactions should have a single fuctionality/responsibility. here we are performing two different function together which a user can always perform separately

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an edge case.

Scenario:
The user want's agoric wallet address. Goes to copy the address but the agoric chain isn't present there ==> Mainly because the user did not include new tokens ==> Output is the user never got the address when they should've gotten it.

Also, assumptions lead to incorrect behaviour and unreliable software.

As for SRP, I am all in for it. But then we should apply it consistently and where it's required.


await playwright.waitAndClickByText(notificationPageElements.copyAddress);
await page.click(notificationPageElements.copyWalletAddressSelector);
await playwright.waitAndClick(notificationPageElements.walletSelectors(chainName))

walletAddress = clipboardy.readSync();
await playwright.switchToCypressWindow();
return walletAddress;
Expand Down Expand Up @@ -287,22 +324,25 @@ const keplr = {

return true;
},

async addNewTokensFound() {
module.exports.switchToKeplrIfNotActive();
await module.exports.goToHome();
async addNewTokensFound(switchScreens = true) {
if (switchScreens) {
module.exports.switchToKeplrIfNotActive();
await module.exports.goToHome();
}
await playwright.waitAndClickByText(homePageElements.newTokensFound);
await playwright.waitAndClick(
await playwright.waitAndClickWithRetry(
homePageElements.selectAllTokensCheck,
playwright.keplrWindow(),
{ number: -1, force: true },
);
await playwright.waitAndClickByText(
homePageElements.addChainsButton,
playwright.keplrWindow(),
true,
);
await playwright.switchToCypressWindow();

if (switchScreens) {
await playwright.switchToCypressWindow();
}
Comment on lines +342 to +345
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should always be switching screens regardless.
currently we're building our flows with the mindset that every interaction/command will switch to the keplr window, perform its interaction, and switch back to the cypress window.
if we start performing interactions that deviate from that, it could break the flow of subsequent interactions

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed to reuse this implementation in a flow I was implementing without the screen-switching part. All the tests were passed and now flow was broken.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what flow was this? i dont see it being used anywhere
the should get the accurate values for the tokens in the wallet spec uses getTokenAmount twice in succession and it doesn't have any issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see the getWalletAddress function


return true;
},
Expand All @@ -319,11 +359,11 @@ const keplr = {
);
const innerTexts = await parentElement.allInnerTexts();
const textArray = innerTexts[0].split('\n');
const tokenValue = Number(textArray[3]);

const tokenValue = textArray[3];
const parsedTokenValue = Number(tokenValue.replace(/,/g, ''));
await playwright.switchToCypressWindow();

return tokenValue;
return parsedTokenValue;
},
};

Expand Down
25 changes: 25 additions & 0 deletions commands/playwright-keplr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ module.exports = {
page = keplrWindow,
) {
try {
await module.exports.waitUntilStable(page);
await page.waitForSelector(selector, { timeout });
return true;
} catch (error) {
Expand Down Expand Up @@ -307,6 +308,30 @@ module.exports = {
await element.type(value);
await module.exports.waitUntilStable(page);
},
async waitAndClickWithRetry(selector, options) {
const maxRetries = 5;
let retries = 0;

while (retries < maxRetries) {
try {
await module.exports.waitAndClick(
selector,
module.exports.keplrWindow(),
options,
);
return;
} catch (error) {
retries++;
frazarshad marked this conversation as resolved.
Show resolved Hide resolved
}
}

throw new Error(`Failed to click element after ${maxRetries} attempts`);
},
async waitAndClickWithDelay(selector, options, delay) {
const page = module.exports.keplrWindow()
await page.waitForTimeout(delay)
await module.exports.waitAndClick(selector, page, options);
},
async switchToKeplrNotification() {
const keplrExtensionData = (await module.exports.getExtensionsData()).keplr;

Expand Down
6 changes: 6 additions & 0 deletions pages/keplr/first-time-flow-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,17 @@ const phraseAccountCreated = 'Account Created!';
const finishButton = 'button[type="button"]';
const textAreaSelector = 'textbox';
const submitPhraseButton = 'button[type="submit"]';
const chainName = 'Agoric local'
const chainNameSelector = 'text=Agoric local'
frazarshad marked this conversation as resolved.
Show resolved Hide resolved
const importButtonSelector = 'button:has-text("Import")'

module.exports.onboardingElements = {
existingWalletButton,
createWalletButton,
importRecoveryPhraseButton,
useRecoveryPhraseButton,
chainNameSelector,
importButtonSelector,
phraseCount24,
phrasePrivateKey,
walletInput,
Expand All @@ -32,4 +37,5 @@ module.exports.onboardingElements = {
finishButton,
textAreaSelector,
submitPhraseButton,
chainName
};
2 changes: 2 additions & 0 deletions pages/keplr/home-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ const tokenParentSelector = '../../../..';
const selectAllTokensCheck = 'input[type="checkbox"]:enabled';
const newTokensFound = 'new token(s) found';
const addChainsButton = 'Add Chains';
const newTokensFoundSelector = 'text=new token(s) found'

module.exports.homePageElements = {
tokenNameLabel,
tokenParentSelector,
selectAllTokensCheck,
newTokensFound,
addChainsButton,
newTokensFoundSelector
};
6 changes: 3 additions & 3 deletions pages/keplr/notification-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
const approveButton = `button`;
const copyAddress = 'Copy Address';
const copyWalletAddressSelector = 'div.sc-dkzDqf div.sc-hKMtZM.sc-kDDrLX.cyoEAq.dkJSBQ'
const walletSelectors = chainName => `img[alt="${chainName}"]`;

module.exports.notificationPageElements = {
approveButton,
copyAddress,
copyWalletAddressSelector
};
walletSelectors
};
5 changes: 3 additions & 2 deletions support/commands.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,11 +455,12 @@ Cypress.Commands.add('disconnectWalletFromDapp', () => {
return cy.task('disconnectWalletFromDapp');
});

Cypress.Commands.add('getWalletAddress', () => {
cy.task('getWalletAddress').then(address => {
Cypress.Commands.add('getWalletAddress', chainName => {
cy.task('getWalletAddress', chainName).then(address => {
return address;
});
});

Cypress.Commands.add('switchWallet', walletName => {
return cy.task('switchWallet', { walletName });
});
Expand Down
20 changes: 17 additions & 3 deletions tests/e2e/specs/keplr/keplr-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,16 @@ describe('Keplr', () => {
expect(setupFinished).to.be.true;
});
});

it(`should get wallet address while running addNewTokensFound flow`, () => {
cy.getWalletAddress('Agoric localhost').then(walletAddress => {
expect(walletAddress.length).to.be.equal(45);
});

cy.getWalletAddress('Cosmos Hub').then(walletAddress => {
expect(walletAddress.length).to.be.equal(45);
});
});
Comment on lines +78 to +87
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rabi-siddique we have two sets of these test cases. both which don't technically run the addNewTokensFound flow. we should fix this in a later PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frazarshad The test cases you're referring to are working fine. addNewTokensFound flow is essential for reliable results.

it(`should switch to new wallet by name`, () => {
cy.switchWallet('My Wallet 2').then(taskCompleted => {
expect(taskCompleted).to.be.true;
Expand All @@ -99,10 +109,14 @@ describe('Keplr', () => {
});
});

it(`should get wallet address`, () => {
cy.getWalletAddress().then(walletAddress => {
it(`should get wallet address without running addNewTokensFound flow`, () => {
cy.getWalletAddress('Agoric localhost').then(walletAddress => {
expect(walletAddress.length).to.be.equal(45);
});

cy.getWalletAddress('Cosmos Hub').then(walletAddress => {
expect(walletAddress.length).to.be.equal(45);
});
});
});
});
});
Loading