Skip to content

Add metrics e2e test#9784

Merged
Gudahtt merged 1 commit intodevelopfrom
metrics-e2e-test
Dec 1, 2020
Merged

Add metrics e2e test#9784
Gudahtt merged 1 commit intodevelopfrom
metrics-e2e-test

Conversation

@Gudahtt
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt commented Nov 3, 2020 •
edited
Loading

An e2e test has been added that uses the new mock Segment server to verify that the three initial page metric events are sent correctly.

Using the mock Segment server requires a special build with this mock Segment server hostname embedded, so a distinct job for building and running this test was required. As such, it was left out of the run-all.sh script.

@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Nov 4, 2020

Well. That's an interesting result! It works every time locally.

@Gudahtt Gudahtt force-pushed the add-mock-segment-server branch 2 times, most recently from f689105 to 91fa3c2 Compare November 7, 2020 00:33
Base automatically changed from add-mock-segment-server to develop November 9, 2020 21:45
@Gudahtt Gudahtt dismissed a stale review November 9, 2020 21:45

The base branch was changed.

@Gudahtt Gudahtt force-pushed the metrics-e2e-test branch 2 times, most recently from 70f4d51 to ed785b2 Compare November 9, 2020 22:24
@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Nov 9, 2020

Huh. I added a log to get some insight into what's happening, and it definitely appears to be failing on CI and working locally. I haven't seen the event get triggered on CI yet, yet it is always triggered when I run it locally, using exactly the same commands.

I'm using a different version of Firefox than CI is 🤔 . But I'm guessing this is just some race condition. I guess there is no guarantee that anything async performed in beforeunload will have time to finish.

@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Nov 11, 2020

I'll update this PR to test something that does work more reliably. We'll have to revisit this beforeunload behavior another time, perhaps by sending this metric event from the background instead.

@Gudahtt Gudahtt force-pushed the metrics-e2e-test branch 2 times, most recently from c5c18eb to 4f73122 Compare November 30, 2020 20:41
@Gudahtt
Copy link
Copy Markdown
Member Author

Gudahtt commented Nov 30, 2020 •
edited
Loading

I have updated this PR (and the description) to test for the first two Page events, rather than for the close event.

Curiously, there is no third page event when the user submits their password, and navigates back to "Home" from "Unlock Page" 🤔

Nevermind, figured it out! I wasn't waiting for the Home page to finish loading. After await-ing the Home page render, all three page events show up correctly.

@Gudahtt
Add metrics e2e test
7bbf94d 
An e2e test has been added that uses the new mock Segment server to
verify that the three initial page metric events are sent correctly.

Using the mock Segment server requires a special build with this mock
Segment server hostname embedded, so a distinct job for building and
running this test was required. As such, it was left out of the
`run-all.sh` script.
@Gudahtt Gudahtt marked this pull request as ready for review November 30, 2020 21:16
@Gudahtt Gudahtt requested review from a team and kumavis as code owners November 30, 2020 21:16
@Gudahtt Gudahtt requested a review from darkwing November 30, 2020 21:16
@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Nov 30, 2020

Builds ready [7bbf94d]
Page Load Metrics (458 ± 46 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3085572311
domContentLoaded2755534569646
load2765544589646
domInteractive2755534569646

darkwing
darkwing approved these changes Dec 1, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@darkwing darkwing left a comment

Choose a reason for hiding this comment

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

Really nicely done @Gudahtt ! The test itself seems to take a while (almost 7 seconds), but that's likely triggered by MM's initial load. Since other tests take longer, I'm not too worried about it. Thank you!

@Gudahtt Gudahtt merged commit cc1161a into develop Dec 1, 2020
@Gudahtt Gudahtt deleted the metrics-e2e-test branch December 1, 2020 21:24
@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@kumavis kumavis Awaiting requested review from kumavis kumavis is a code owner

1 more reviewer

@darkwing darkwing darkwing approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@darkwing darkwing

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Gudahtt @metamaskbot @darkwing