Use JSON onboarding in e2e tests of transactions #3559
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Prior to this change we were onbording using a seed phrase, which was not ideal, as it could lead to a seed phrase leakage in the Playwright report added to the artifacts of CI jobs (that's why we've temporarily switched off storing of the test artifacts). Now that we support in Taho the onboarding with a JSON keystore file (which stores an encrypted private key), we are good to use this method of account import in our E2E tests. This method is safer, as the sensitive data entered in the form during onboarding is obfuscated, so there's no risk it will leak in the Playwright report (and even if so, what will leak will be just a password, not a corresponding to it JSON file). As we're switching to this onboarding method, we can bring back storing of the Playwrigt reports in the CI jobs artifacts. Apart from the above, as part of this commit/PR we're also introducing a script which can be used to generate a JSON keystore file based on the provided private key and password.
| @@ -0,0 +1,17 @@ | |||
| /* eslint-disable @typescript-eslint/no-var-requires */ | |||
There was a problem hiding this comment.
I decided to add a linter exception here, as I couldn't make the linter happy without braking the script (can't say confidently that this can't be done though...).
There was a problem hiding this comment.
Choice looks correct, just drop a note above it that since this is a script we can’t use import yet.
|
Hmm, I don't why the lint check is failing... When I run |
Shadowfiend
reviewed
Jul 20, 2023
| @@ -0,0 +1,17 @@ | |||
| /* eslint-disable @typescript-eslint/no-var-requires */ | |||
| const fs = require("fs") | |||
| const wallet = require("ethereumjs-wallet").default | |||
There was a problem hiding this comment.
For the lint error below—we need to yarn install in the parent dir in the actions workflow.
There was a problem hiding this comment.
Added in 24b20bc, it helped, thanks!
We need to run the install there to avoid complaints from the linter about a missing module (`Unable to resolve path to module 'ethereumjs-wallet'`).
The transactions e2e tests use the first address of the testing wallet (`testertesting.eth`). But we already know that in some tests that we plan to add in the future, we'll want to use a different account - the third address of the testing wallet (`e2e.testertesting.eth`). So we will need to use names of the variables and secrets that differenciate those different addresses.
I've pushed by mistake a change commenting out some fragment of the e2e tests that shouldn't be commented out. I'm fixing it in this commit.
michalinacienciala
force-pushed
the
json-import-in-e2e
branch
from
e8004d8
to
3e22845
Compare
|
@Shadowfiend, ready for an another round of review. |
We changed this for production fork builds to differentiate from the release builds, and we updated e2e tests to look for the suffix, but on non release branches the fork builds were not getting the suffix, leading to test failures.
|
The fork build still seems to be failing here, so once we get that sorted we'll be good to go. Which is good because the main e2e tests are failing atm 😅 |
5 tasks
|
@Shadowfiend, since merging the Let's remove |
|
Ok, I think we're good here. Going to see if the e2e tests run as expected and then will land this fellow. |
Shadowfiend
approved these changes
Jul 25, 2023
Merged
jagodarybacka
added a commit
that referenced
this pull request
Aug 2, 2023
## What's Changed * v0.43.3 by @hyphenized in #3570 * Use JSON onboarding in e2e tests of transactions by @michalinacienciala in #3559 * Refactor prices caching by @hyphenized in #3526 * Fix assertions made on the latest transaction by @michalinacienciala in #3573 **Full Changelog**: v0.43.3...v0.44.0 Latest build: [extension-builds-3577](https://github.com/tahowallet/extension/suites/14645396551/artifacts/830824018) (as of Fri, 28 Jul 2023 14:43:48 GMT).
hyphenized
added a commit
that referenced
this pull request
Aug 16, 2023
This commit introduces E2E test that checks the functiolality of verified/unverified tokens. Following general steps are executed: 1. Import account 2. Enable `Show unverified assets` 3. Hide asset 4. Trust asset 5. Hide trusted asset A number of checks is performed during each step. The commit also introduces helper functions and adds `data-testid` attribute to a couple of DOM elements. TODO: - [x] As some of the code is similar or identical as in the #3418 PR which is yet not merged to `main`, some conflicts may arise and will need to be resolved before this change lands on `main`. - [x] Also some changes will need to be made once #3195 gets merged to `main`, as this PR solves a bug causing failures in the tests (the failing part of the test was temporarily commented out). - [x] Add `E2E_TEST_ONLY_WALLET_JSON_BODY` and `E2E_TEST_ONLY_WALLET_JSON_PASSWORD` secrets in GitHub's settings. - [x] Wait for #3559 to be merged and merge main to feature branch (should fix the failing `e2e-tests` job) - [x] Handle TODOs in the code Latest build: [extension-builds-3472](https://github.com/tahowallet/extension/suites/15116290312/artifacts/863816729) (as of Tue, 15 Aug 2023 18:12:30 GMT).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this change we were onboarding using a seed phrase, which was not ideal, as it could lead to a seed phrase leakage in the Playwright report added to the artifacts of CI jobs (that's why we've temporarily switched off storing of the test artifacts).
Now that we support in Taho the onboarding with a JSON keystore file (which stores an encrypted private key), we are good to use this method of account import in our E2E tests. This method is safer, as the sensitive data entered in the form during onboarding is obfuscated, so there's no risk it will leak in the Playwright report (and even if so, what will leak will be just a password, not a corresponding to it JSON file).
As we're switching to this onboarding method, we can bring back storing of the Playwright reports in the CI jobs artifacts.
Apart from the above, as part of this commit/PR we're also introducing a script which can be used to generate a JSON keystore file based on the provided private key and password.
TODO:
TEST_WALLET_JSON_BODYandTEST_WALLET_JSON_PASSWORDsecrets in the GitHub SettingsTEST_WALLET_RECOVERY_PHRASEsecretLatest build: extension-builds-3559 (as of Tue, 25 Jul 2023 19:09:50 GMT).