test: migrate simple send spec to POM

[seaona]

Description

Related issues

Fixes:

Manual testing steps

1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

• I’ve followed MetaMask Coding Standards.
• I've clearly explained what problem this PR is solving and how it is solved.
• I've linked related issues
• I've included manual testing steps
• I've included screenshots/recordings if applicable
• I’ve included tests if applicable
• I’ve documented my code using JSDoc format if applicable
• I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
• I’ve properly set the pull request status:
  • In case it's not yet "ready for review", I've set it to "draft".
  • In case it's "ready for review", I've changed it from "draft" to "non-draft".

Pre-merge reviewer checklist

• I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
• I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

[github-actions]

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

[seaona]

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

[seaona]

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

[seaona]

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

[seaona]

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

[seaona]

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

[seaona]

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

[chloeYue]

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.

[seaona]

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

[chloeYue]

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.

[chloeYue]

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.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

[github-actions]

This PR was closed because there has been no follow up activity in the last 14 days. Thank you for your contributions.