fix: flaky test `Vault Decryptor Page is able to decrypt the vault us..` due to empty file load #26612

seaona · 2024-08-22T13:24:40Z

Description

The vault decrypt test is flaky, at the point where we upload the vault file into the browser.
I added logs for checking the file size, and I saw that when the file failed to upload, the file size was really small.

The problem seemed to start appearing after this PR was merged. The changes doesn't seem related, but it updated a lot of dependencies, some of them related to cryptography/addres derivation packages (see screenshot below) - which could somehow affect somehow the timings.

Fix:

Added a function to get the file size
Added a retry logic to check the file size before uploading it, making sure it's bigger than a conservatory size

See logic in action:

Notes and future work:

there isn't an easy way to determine when Chrome has finished writing into the file, since this can depend on numerous factors ( Data Changes, Internal Buffering, Extension Activity, Browser Events..)
currently we are testing the log file, instead of the ldb file, as described in the Vault Decryptor instructions. I haven't find the ldb file, after waiting ~30seconds and closing/repoening the browser, however, that file might also exibit flakiness as Chrome would also need to write to it. For this reason, in this PR the test is kept as it was, and it only focuses on fixing the spec with the retry logic.
there is another test proposed by @davidmurdoch , for reading the vault value from the file and then using the function "Paste into the field" for testing that. I've decided to do this on a separate PR and keep this PR focused on the current issue only, to unblock ci faster

Related issues

Fixes: #26572

Manual testing steps

Check ci https://app.circleci.com/jobs/github/MetaMask/metamask-extension/3612121

Screenshots/Recordings

When the error appears, we can see how the file size is really small 19 bytes.

whereas when the test is successful, the file size is bigger 7358117 bytes.

See changes mentioned, after flakiness started appearing:

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
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.

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 · 2024-08-22T13:24:54Z

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.

codecov · 2024-08-22T13:45:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.09%. Comparing base (b3865bc) to head (7f9762f).
Report is 15 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #26612      +/-   ##
===========================================
+ Coverage    70.04%   70.09%   +0.05%     
===========================================
  Files         1411     1413       +2     
  Lines        49205    49254      +49     
  Branches     13754    13768      +14     
===========================================
+ Hits         34463    34524      +61     
+ Misses       14742    14730      -12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

metamaskbot · 2024-08-22T13:58:07Z

Builds ready [7548864]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (77 ± 11 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	60	176	105	30	14
		domContentLoaded	40	140	73	23	11
		load	46	140	77	23	11
		domInteractive	9	76	32	19	9

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

metamaskbot · 2024-08-22T14:53:34Z

Builds ready [aeba870]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (82 ± 9 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	60	157	111	25	12
		domContentLoaded	47	117	79	20	10
		load	48	117	82	19	9
		domInteractive	10	66	32	14	7

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

socket-security · 2024-08-22T17:51:39Z

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

yarn.lock

chloeYue · 2024-08-23T11:28:39Z

test/e2e/vault-decryption-chrome.spec.js

+        });
+
+        // Retry-logic if file upload fails
+        if (!fileLoaded) {


Hi, I have a question about the logic. The value of the fileLoaded variable will be true if the element "Can not read vault from file" appears. Is this intended? It seems like it should be reversed. But maybe I misunderstood.

hey @chloeYue that's an awesome catch 😍 I've pushed a fix for that.
I'm now trying to see if there could be an alternative way of fixing this on the root cause - based on [this comment] from David (https://consensys.slack.com/archives/CTQAGKY5V/p1724395549815109?thread_ts=1724321902.861359&cid=CTQAGKY5V) If so, I'll update the PR one more time, if not, then we can leave this as a mitigation. I'll share my updates soon 🙏

test/e2e/vault-decryption-chrome.spec.js

sonarqubecloud · 2024-08-23T11:49:34Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

metamaskbot · 2024-08-23T12:18:29Z

Builds ready [5621c96]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (81 ± 8 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	74	142	106	19	9
		domContentLoaded	50	118	76	17	8
		load	57	118	81	17	8
		domInteractive	11	65	29	13	6

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

sonarqubecloud · 2024-08-26T09:13:41Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

metamaskbot · 2024-08-26T09:47:06Z

Builds ready [7f9762f]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (74 ± 9 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	68	151	96	20	10
		domContentLoaded	48	117	67	21	10
		load	48	117	74	19	9
		domInteractive	9	63	24	13	6

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

Gudahtt

LGTM!

chloeYue

LGTM !

checking race condition point

7548864

github-actions bot added the team-extension-platform label Aug 22, 2024

add retry logic and log

a2dc87b

seaona self-assigned this Aug 22, 2024

seaona changed the title ~~fix: draft~~ fix: flaky test Vault Decryptor Page is able to decrypt the vault us.. due to empty file load Aug 22, 2024

move back config

aeba870

seaona marked this pull request as ready for review August 22, 2024 14:24

seaona requested a review from a team as a code owner August 22, 2024 14:24

seaona added the area-qa Relating to QA work (Quality Assurance) label Aug 22, 2024

seaona requested review from kumavis and a team as code owners August 22, 2024 17:50

legobeat reviewed Aug 23, 2024

View reviewed changes

yarn.lock Outdated Show resolved Hide resolved

seaona force-pushed the test-vault-file branch from a59a0f1 to aeba870 Compare August 23, 2024 05:58

seaona requested a review from legobeat August 23, 2024 05:59

chloeYue reviewed Aug 23, 2024

View reviewed changes

seaona commented Aug 23, 2024

View reviewed changes

test/e2e/vault-decryption-chrome.spec.js Outdated Show resolved Hide resolved

test/e2e/vault-decryption-chrome.spec.js Outdated Show resolved Hide resolved

seaona added 2 commits August 23, 2024 13:47

Update test/e2e/vault-decryption-chrome.spec.js

17af063

Update test/e2e/vault-decryption-chrome.spec.js

5621c96

seaona added 3 commits August 26, 2024 11:06

retry file size before upload

ffae80d

add back comment

6558951

fix lint

7f9762f

Gudahtt approved these changes Aug 26, 2024

View reviewed changes

chloeYue approved these changes Aug 26, 2024

View reviewed changes

seaona merged commit 80b28d6 into develop Aug 26, 2024
84 checks passed

seaona deleted the test-vault-file branch August 26, 2024 15:35

github-actions bot locked and limited conversation to collaborators Aug 26, 2024

metamaskbot added the release-12.5.0 Issue or pull request that will be included in release 12.5.0 label Aug 26, 2024

seaona added the flaky tests label Aug 30, 2024

gauthierpetetin added release-12.4.0 Issue or pull request that will be included in release 12.4.0 and removed release-12.5.0 Issue or pull request that will be included in release 12.5.0 labels Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: flaky test `Vault Decryptor Page is able to decrypt the vault us..` due to empty file load #26612

fix: flaky test `Vault Decryptor Page is able to decrypt the vault us..` due to empty file load #26612

seaona commented Aug 22, 2024 •

edited

Loading

github-actions bot commented Aug 22, 2024

codecov bot commented Aug 22, 2024 •

edited

Loading

metamaskbot commented Aug 22, 2024

metamaskbot commented Aug 22, 2024

socket-security bot commented Aug 22, 2024 •

edited

Loading

chloeYue Aug 23, 2024

seaona Aug 23, 2024

sonarqubecloud bot commented Aug 23, 2024

metamaskbot commented Aug 23, 2024

sonarqubecloud bot commented Aug 26, 2024

metamaskbot commented Aug 26, 2024

Gudahtt left a comment

chloeYue left a comment

fix: flaky test Vault Decryptor Page is able to decrypt the vault us.. due to empty file load #26612

fix: flaky test Vault Decryptor Page is able to decrypt the vault us.. due to empty file load #26612

Conversation

seaona commented Aug 22, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Aug 22, 2024

codecov bot commented Aug 22, 2024 • edited Loading

Codecov Report

metamaskbot commented Aug 22, 2024

metamaskbot commented Aug 22, 2024

socket-security bot commented Aug 22, 2024 • edited Loading

chloeYue Aug 23, 2024

Choose a reason for hiding this comment

seaona Aug 23, 2024

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 23, 2024

Quality Gate passed

metamaskbot commented Aug 23, 2024

sonarqubecloud bot commented Aug 26, 2024

Quality Gate passed

metamaskbot commented Aug 26, 2024

Gudahtt left a comment

Choose a reason for hiding this comment

chloeYue left a comment

Choose a reason for hiding this comment

fix: flaky test `Vault Decryptor Page is able to decrypt the vault us..` due to empty file load #26612

fix: flaky test `Vault Decryptor Page is able to decrypt the vault us..` due to empty file load #26612

seaona commented Aug 22, 2024 •

edited

Loading

codecov bot commented Aug 22, 2024 •

edited

Loading

socket-security bot commented Aug 22, 2024 •

edited

Loading