-
Notifications
You must be signed in to change notification settings - Fork 26
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
chore: Replace old onboarding metrics with Amplitude events #3769
chore: Replace old onboarding metrics with Amplitude events #3769
Conversation
src/services/events/types.ts
Outdated
loginProvider?: ReturnType<typeof loginProviderToName> // for login buttons only | ||
ciProvider?: 'GitHub Actions' | 'CircleCI' | 'Codecov CLI' // E.g., product onboarding pages | ||
testingFramework?: Framework // E.g., product onboarding pages | ||
copied?: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could see an argument for using a string type here rather than string literal union. Lmk thoughts if you have any
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put these in an enum and infer the type from there? similar to loginProvider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulled it out to its own type. Same result, but don't think we need the extra enum features
Bundle ReportChanges will decrease total bundle size by 2.73kB (-0.02%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: gazebo-production-systemAssets Changed:
Files in
Files in
view changes for bundle: gazebo-production-esmAssets Changed:
Files in
Files in
|
Bundle ReportChanges will decrease total bundle size by 2.73kB (-0.02%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: gazebo-staging-systemAssets Changed:
Files in
Files in
view changes for bundle: gazebo-staging-esmAssets Changed:
Files in
Files in
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3769 +/- ##
==========================================
+ Coverage 98.68% 98.72% +0.03%
==========================================
Files 825 824 -1
Lines 14831 14782 -49
Branches 4242 4239 -3
==========================================
- Hits 14636 14593 -43
+ Misses 188 182 -6
Partials 7 7
... and 12 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #3769 +/- ##
==========================================
+ Coverage 98.68% 98.72% +0.03%
==========================================
Files 825 824 -1
Lines 14831 14782 -49
Branches 4250 4239 -11
==========================================
- Hits 14636 14593 -43
+ Misses 188 182 -6
Partials 7 7
... and 12 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3769 +/- ##
==========================================
+ Coverage 98.68% 98.72% +0.03%
==========================================
Files 825 824 -1
Lines 14831 14782 -49
Branches 4242 4239 -3
==========================================
- Hits 14636 14593 -43
+ Misses 188 182 -6
Partials 7 7
... and 12 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3769 +/- ##
==========================================
+ Coverage 98.68% 98.72% +0.03%
==========================================
Files 825 824 -1
Lines 14831 14782 -49
Branches 4242 4239 -3
==========================================
- Hits 14636 14593 -43
+ Misses 188 182 -6
Partials 7 7
... and 12 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
await waitFor(() => | ||
expect(mockMetricMutationVariables).toHaveBeenCalledTimes(1) | ||
) | ||
await waitFor(() => expect(eventTracker().track).toHaveBeenCalledTimes(4)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how come this is 4 and not 5 when there's 5 copy commands? :O
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
const promises: Promise<void>[] = [] | ||
copyCommands.forEach((copy) => promises.push(user.click(copy))) | ||
await Promise.all(promises) | ||
|
||
// One of the code-snippets does not have a metric associated with it | ||
expect(mockMetricMutationVariables).toHaveBeenCalledTimes(4) | ||
expect(eventTracker().track).toHaveBeenCalledTimes(5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar here with the off by one
b0df190
to
db5123e
Compare
…xactly the way having typesafety fixes lol)
@@ -92,5 +96,23 @@ type ButtonName = | |||
| 'Continue' | |||
| 'Login' | |||
| 'Sync' | |||
| 'Copy' | |||
|
|||
type ButtonLocation = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this type because I accidentally used both Coverage onboarding
and Coverage Onboarding
. I expected these strings to be more or less unique to each call, but that seems to not be the case. If we're reusing locations, we ought to have type safety to avoid mistakes like mine.
This PR removes the old onboarding metrics implementation replacing with Amplitude where applicable.
Closes codecov/engineering-team#3125