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

test: migrate simple send spec to POM #22464

Closed
wants to merge 1 commit into from
Closed

test: migrate simple send spec to POM #22464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 14 additions & 0 deletions test/e2e/components/header-navbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
class HeaderNavbar {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this component is not used in the simple-send spec, but is added in order to build the skeleton of the Page Object Model patterns defined in the proposal. It will be used in other specs and can be expanded according to the specific needs.
Also more components can be added following this example

constructor(driver) {
// Selectors
this.driver = driver;
this.btnAccountMenu = '[data-testid="account-menu-icon"]';
}

// Methods
async openAccountMenu() {
await this.driver.clickElement(this.btnAccountDetailsMenu);
}
}

module.exports = HeaderNavbar;
10 changes: 10 additions & 0 deletions test/e2e/page-objects/base.page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
const HeaderNavbar = require('../components/header-navbar');

class BasePage {
constructor(driver) {
this.driver = driver;
this.headerNavbar = new HeaderNavbar(driver);
}
}

module.exports = BasePage;
16 changes: 16 additions & 0 deletions test/e2e/page-objects/confirm-tx.page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
const BasePage = require('./base.page');

class ConfirmTxPage extends BasePage {
constructor(driver) {
// Selectors
super(driver);
this.btnConfirm = '[data-testid="page-container-footer-next"]';
}

// Methods
async confirmTx() {
await this.driver.clickElement(this.btnConfirm);
}
}

module.exports = ConfirmTxPage;
26 changes: 26 additions & 0 deletions test/e2e/page-objects/home.page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
const BasePage = require('./base.page');

class HomePage extends BasePage {
constructor(driver) {
// Selectors
super(driver);
this.btnSend = '[data-testid="eth-overview-send"]';
this.btnActivity = '[data-testid="home__activity-tab"]';
this.confirmedTx = '.transaction-status-label--confirmed';
}

// Methods
async startSendFlow() {
await this.driver.clickElement(this.btnSend);
}

async goToActivityList() {
await this.driver.clickElement(this.btnActivity);
}

async isConfirmedTxInActivity() {
return await this.driver.isElementPresent(this.confirmedTx);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

notice here we don't do any assertion, we only return a boolean. As stated in the guidelines, assertions should go on the spec level, not on the page object classes. See below how this method is called in the spec for doing the assertion there

Copy link
Contributor

Choose a reason for hiding this comment

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

I propose we consider incorporating assertion methods within our page object. This approach could offer several advantages:
Code Reusability: By creating methods like check_confirmedTxDisplayedInActivity() and check_confirmedTxNotDisplayedInActivity(), we can reuse these assertions throughout our tests, saving coding time.

async check_confirmedTxDisplayedInActivity() {
  assert.equal(await isConfirmedTxInActivity(), true, 'Confirmed tx is not found');
}
async check_confirmedTxNotDisplayedInActivity() {
assert.equal(await isConfirmedTxInActivity(), false, 'Confirmed tx is displayed');
}

For the class organization: We already have elements and action methods, when we add a section for check methods in the page class, we could consider adopting a naming convention such as check_xxx for assertions and action_xxx for actions. Something like action_ startSendFlow(), This approach would make it easier to locate specific methods within the class, especially when dealing with a large list of class methods
We can also add parameter for check methods, which allowing us to adapt the error message dynamically. For example:

async check_confirmedTxNumber(expectedNumberOfTx) {
  let currentNumberOfTx = txElements.length;
  assert.equal(
    expectedNumberOfTx, 
    currentNumberOfTx, 
    `We have ${expectedNumberOfTx} transactions appear instead of ${currentNumberOfTx}`
  );
}

This approach would automatically generate a helpful error message for debugging purposes. In fact, I would recommend that all check methods have their error messages defined within the class. Currently, in our tests, error messages are not always specified in the test spec. By defining error messages within the class, we can ensure that meaningful error messages are consistently used throughout our tests, making it easier for people to create and debug tests.

}

module.exports = HomePage;
21 changes: 21 additions & 0 deletions test/e2e/page-objects/login.page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
const BasePage = require('./base.page');

class LoginPage extends BasePage {
constructor(driver) {
// Selectors
super(driver);
this.inputPassword = '#password';
this.btnUnlock = '[data-testid="unlock-submit"]';
}

// Methods
async addPassword(password) {
await this.driver.fill(this.inputPassword, password);
}

async unlock() {
await this.driver.clickElement(this.btnUnlock);
}
}

module.exports = LoginPage;
21 changes: 21 additions & 0 deletions test/e2e/page-objects/send-amount.page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
const BasePage = require('./base.page');

class SendToPage extends BasePage {
constructor(driver) {
// Selectors
super(driver);
this.inputAmount = '[data-testid="currency-input"]';
this.btnNext = '[data-testid="page-container-footer-next"]';
}

// Methods
async addAmount(amount) {
await this.driver.fill(this.inputAmount, amount);
}

async goToNextScreen() {
await this.driver.clickElement(this.btnNext);
}
}

module.exports = SendToPage;
16 changes: 16 additions & 0 deletions test/e2e/page-objects/send-to.page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
const BasePage = require('./base.page');

class SendToPage extends BasePage {
constructor(driver) {
// Selectors
super(driver);
this.inputRecipient = '[data-testid="ens-input"]';
}

// Methods
async addRecipient(recipientAddress) {
await this.driver.fill(this.inputRecipient, recipientAddress);
}
}

module.exports = SendToPage;
11 changes: 11 additions & 0 deletions test/e2e/processes/unlock-wallet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
const LoginPage = require('../page-objects/login.page');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

processes: are flows that keep repeating across different tests with few variations (captured by params in the functions)
This is the most simple unlock wallet process, but there are more that could be captured around that (in helpers file, we have logInWithBalanceValidation unlockWallet with a couple of variations - to be included in the future


const unlockWallet = async (driver, password) => {
const loginPage = new LoginPage(driver);

await driver.navigate();
await loginPage.addPassword(password);
await loginPage.unlock();
};

module.exports = unlockWallet;
33 changes: 26 additions & 7 deletions test/e2e/tests/simple-send.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
const { strict: assert } = require('assert');
const {
defaultGanacheOptions,
withFixtures,
sendTransaction,
logInWithBalanceValidation,
WALLET_PASSWORD,
} = require('../helpers');
const FixtureBuilder = require('../fixture-builder');
const ConfirmTxPage = require('../page-objects/confirm-tx.page');
const HomePage = require('../page-objects/home.page');
const SendToPage = require('../page-objects/send-to.page');
const SendAmountPage = require('../page-objects/send-amount.page');
const unlockWallet = require('../processes/unlock-wallet');

describe('Simple send', function () {
it('can send a simple transaction from one account to another', async function () {
Expand All @@ -14,14 +19,28 @@ describe('Simple send', function () {
ganacheOptions: defaultGanacheOptions,
title: this.test.fullTitle(),
},
async ({ driver, ganacheServer }) => {
await logInWithBalanceValidation(driver, ganacheServer);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this method is at the moment defined in helpers file and it has a flavor of a process - this has been defined as the process unlock-wallet as a very simple process - it should be expanded to match the different login options we have

async ({ driver }) => {
// Process
await unlockWallet(driver, WALLET_PASSWORD);

await sendTransaction(
Copy link
Contributor Author

@seaona seaona Jan 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this method is at the moment defined in helpers file and it has a flavor of a process - understood in the POM context. The decision here was to, instead of directly implement a process for send-transaction (which we will very likely need in the future) just to implement the steps one by one in the test.
When to add a process? if the group of steps is repeated in another tests then, we could abstract it as a process (like done in unlock wallet above).

The approach of not going over a process here serves as a demo purpose, as we can see here both processes and normal page object actions using their methods in the specs

driver,
// Page Objects and Components
const confirmTxPage = new ConfirmTxPage(driver);
const homePage = new HomePage(driver);
const sendAmountPage = new SendAmountPage(driver);
const sendToPage = new SendToPage(driver);

// Actions and Assertions
await homePage.startSendFlow();
await sendToPage.addRecipient(
'0x985c30949c92df7a0bd42e0f3e3d539ece98db24',
'1',
);
await sendAmountPage.addAmount('1');
Copy link
Contributor

@chloeYue chloeYue Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we want to keep page objects independent, then when we switch page, we don't return the object of the new class. To improve tests stability, i suggest we add waitPageLoaded() method to each class, so when we switch page in test spec, we can call the waitPageLoaded() first to be sure that we are on the expected page and ready for actions.
Another option is to allow dependency, then we can do something like:

async startSendFlow() {
    await this.driver.clickElement(this.btnSend);
    await (new SendToPage(driver)). waitSendToPageLoaded();
 }

The advantage is that we don't need to add waitPageLoaded() everywhere in the test body.

await sendAmountPage.goToNextScreen();
await confirmTxPage.confirmTx();
await homePage.goToActivityList();
Copy link
Contributor Author

@seaona seaona Jan 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

all the actions we perform in the test should be clear by their name. This can also be seen as documentation of flows. From this test we infer that Simple Send flow basically is:

  1. login
  2. start a send flow
  3. add a recipient address
  4. add a token amount
  5. proceed to the next screen
  6. confirm transaction
  7. go to activity list
  8. (in the assertion) check tx is there


const confirmedTx = await homePage.isConfirmedTxInActivity();
assert.equal(confirmedTx, true, 'Confirmed tx is not found');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

assertions are always done on the spec level (never on Page Objects)

},
);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

As we're migrating to TypeScript, it might be an ideal time to also adopt it when we implementing the POM. I suggest we consider writing our code in TypeScript during this process.

Expand Down
Loading