-
Notifications
You must be signed in to change notification settings - Fork 639
feat: Add snap_startTrace and snap_endTrace support to snaps-jest
#3547
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3547 +/- ##
=======================================
Coverage 98.27% 98.28%
=======================================
Files 415 417 +2
Lines 11717 11765 +48
Branches 1821 1827 +6
=======================================
+ Hits 11515 11563 +48
Misses 202 202 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| }); | ||
| } | ||
|
|
||
| case 'trace': { |
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.
I had to add this because we clear the trackables state for every request. We could consider not clearing performance traces between requests as well.
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.
Would it be a lot of work to keep pendingTraces between runs?
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.
Unsure whether most usage would be during a single request tbh 🤔
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.
We explicitly clear it, so it would be easy to keep it. Just unsure about the expectations.
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.
Do we have no existing state between requests?
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.
Right now all the notifications and trackables are cleared:
snaps/packages/snaps-simulation/src/request.ts
Lines 104 to 105 in f08611a
| store.dispatch(clearNotifications()); | |
| store.dispatch(clearTrackables()); |
But Snap state isn't.
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.
Hmm. Up to you whether you think it makes sense to persist it between runs, we can always easily change the behavior.
Co-authored-by: Frederik Bolding <[email protected]>
Co-authored-by: Frederik Bolding <[email protected]>
| }, | ||
| endTrace: (state, action: PayloadAction<EndTraceParams>) => { | ||
| const endTrace = castDraft(action.payload); | ||
| const index = state.pendingTraces.findIndex( |
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.
Do we need logic to handle multiple pending traces with the same name/id?
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.
Other than changing it to LIFO like Cursor suggested, I'm not sure what we should change. Unless you think new traces should override existing ones if the name/ID is the same? I have no idea how Sentry handles it, but generally I would expect that Snaps don't run multiple traces at the same time with the same name.
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.
Do we need to match on both name and ID? Are we expecting that Snaps will pass both (our example does not)?
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.
The name is required, but ID is optional. For matching, just passing a name in both cases will work (since undefined === undefined), but if the ID in the end request is different it will fail.
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.
Perhaps fair enough to expect the same parameters on both sides
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.
But I think there is a chance we have to adjust this based on usage
| clearTrackables: (state) => { | ||
| state.events = []; | ||
| state.errors = []; | ||
| state.traces = []; |
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.
| state.traces = []; | |
| state.traces = []; | |
| // We are not resetting `pendingTraces` to let them persist between requests. |
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.
Not sure if this is necessary since it's documented in the test as well
This adds support for
snap_startTraceandsnap_endTracetosnaps-jest, and atoTracematcher to go with it.