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.

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

[UPDATE] Extend the readme documentation to cover E2E testing in more detail. #6210

Merged
merged 5 commits into from
Apr 23, 2023

Conversation

cortisiko
Copy link
Member

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description

For our engineers to contribute to our E2E tests effectively, it's vital that they know how to run them. The goal of this task is to ensure that the readme provides a clear explanation on how to build and execute our automation tests.

Screenshots/Recordings

If applicable, add screenshots and/or recordings to visualize the before and after of your change

Issue

Progresses https://github.com/MetaMask/mobile-planning/issues/895

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@cortisiko cortisiko requested a review from a team as a code owner April 19, 2023 00:49
@cortisiko cortisiko changed the title [UPDATE] Extend the readme documentation to cover automation in more detail. [UPDATE] Extend the readme documentation to cover E2E testing in more detail. Apr 19, 2023
@cortisiko cortisiko added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) team-mobile-client and removed team-mobile-client labels Apr 19, 2023
Copy link
Contributor

@tommasini tommasini left a comment

Choose a reason for hiding this comment

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

LGTM!

README.md Outdated Show resolved Hide resolved
Co-authored-by: tommasini <46944231+tommasini@users.noreply.github.com>
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM! I had one minor concern about the release build instructions, but everything else looks great. Thanks for documenting this, this is super helpful

@NicholasEllul NicholasEllul self-requested a review April 19, 2023 17:33
Copy link
Contributor

@NicholasEllul NicholasEllul left a comment

Choose a reason for hiding this comment

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

Left a comment related to issues starting up with android, currently going through the iOS steps and will leave another review shortly

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@NicholasEllul NicholasEllul left a comment

Choose a reason for hiding this comment

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

One last note:

When running android tests I fail the wallet creation process in the wdio runner.

This is due to the fact that we require an env variable to be set in order for the login to work successfully:

static getValidAccount() {
return {
// A correct BIP39 SRP that can be used for testing. Requires the var to be set in the environment.
seedPhrase: process.env.MM_TEST_ACCOUNT_SRP || 'undefined SRP env var',
// Ethereum address for 1st account of derived on the seed that can be used for testing. Requires the var to be set in the environment.
address:
process.env.MM_TEST_ACCOUNT_ADDRESS || 'undefined address env var',
password: CORRECT_PASSWORD,
};
}

Onboarding steps should indicate that these variables need to be set

Screenshot 2023-04-19 at 3 43 57 PM

Update: I now see that this field is included in a more recent version of the .js.env.example file

@Gudahtt
Copy link
Member

Gudahtt commented Apr 19, 2023

Currently the SRP environment variable is challenging to work with because the test process doesn't source the .js.env file. It won't work unless you source it yourself. It also gets cached by Jest, so it can fail unless you clear your Jest cache.

As a temporary workaround, I'd recommend running this in your terminal before running the e2e tests, and again each time you update that SRP:

yarn jest --clearCache && source .js.env

I would suggest that this be documented in the README as well, except for the fact that this won't be needed anymore once this PR is merged: #6190

@cortisiko
Copy link
Member Author

Thanks for flagging #6190 @Gudahtt, I will wait until the pull request is merged instead of including a workaround in the documentation.

Copy link
Contributor

@NicholasEllul NicholasEllul left a comment

Choose a reason for hiding this comment

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

Looks great!

@cortisiko cortisiko removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Apr 20, 2023
@cortisiko cortisiko merged commit 4655a2a into main Apr 23, 2023
14 checks passed
@cortisiko cortisiko deleted the e2e/fix-documentation branch April 23, 2023 04:47
@github-actions github-actions bot locked and limited conversation to collaborators Apr 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants