test(vue): Add tests for Vue tracing mixins #16486
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds tests for the Vue tracing mixins to improve confidence while continuing development.
- Introduces tests for creating mixins with default and custom lifecycle hooks
- Covers root component behavior, component span lifecycle, and tracking configuration options
Comments suppressed due to low confidence (1)
packages/vue/test/tracing/tracingMixin.test.ts:131
- Consider addressing the todo/fixme by clarifying the intended behavior of the root span when trackComponents is false, or by updating the test to cover both scenarios.
// todo/fixme: This root span is only finished if trackComponents is true --> it should probably be always finished
| }); | ||
|
|
||
| it('should finish root span on timer after component spans end', () => { | ||
| // todo/fixme: This root span is only finished if trackComponents is true --> it should probably be always finished |
There was a problem hiding this comment.
maybe someone knows more here about the wanted behavior 🤔
There was a problem hiding this comment.
hmm this looks like a bug then? according to this code trackComponents shouldn't influence the root component span. Feel free to fix directly or follow up with another PR. Whatever you prefer :) (let me know if I should fix it in case you're booked, also fine!)
There was a problem hiding this comment.
Yeah, it's always starting the span, but not ending it
There was a problem hiding this comment.
Thanks for adding these tests! good to have this covered 🙏
My only ask would be to avoid confusion around "root span" vs. "root component span". We both know that "root span" here refers to the spans for the root component but the next person might confuse this with the actual trace root span.
| }); | ||
|
|
||
| describe('Root Component Behavior', () => { | ||
| it('should always create root span for root component regardless of tracking options', () => { |
There was a problem hiding this comment.
l: Just to avoid confusion: We're talking about a span for the root component, right? Not to confuse with a root (i.e. parentless) span?
| it('should always create root span for root component regardless of tracking options', () => { | |
| it('should always create root component span for root component regardless of tracking options', () => { |
| it('should always create root span for root component regardless of tracking options', () => { | ||
| const mixins = createTracingMixins({ trackComponents: false }); | ||
|
|
||
| mixins.beforeMount.call(mockRootInstance); |
There was a problem hiding this comment.
q (for my understanding mostly): What happens if I call something like beforeCreate or another hook coming in prior to the mount hook? the span would end if the -ed hook emits, right?
There was a problem hiding this comment.
Each operation (create, mount, ...) has its own independent span that is stored in the component's $_sentrySpans object. And only if this operation is stored in there (like create) it will end only if the created is emitted and the create span is available in the $_sentrySpans object.
| it('should respect tracking configuration options', () => { | ||
| // Test different tracking configurations with the same component | ||
| const runTracingTest = (trackComponents: boolean | string[] | undefined, shouldTrack: boolean) => { | ||
| vi.clearAllMocks(); | ||
| const mixins = createTracingMixins({ trackComponents }); | ||
| mixins.beforeMount.call(mockVueInstance); | ||
|
|
||
| if (shouldTrack) { | ||
| expect(startInactiveSpan).toHaveBeenCalled(); | ||
| } else { | ||
| expect(startInactiveSpan).not.toHaveBeenCalled(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
l: It feels like we're running multiple scenarios within one test here which I think we can better illustrate by using it.each and/or splitting tests up a bit. For example, two scenarios should lead to a span being started, while two should not. Those are already two different branches.
The two advantages of running individual tests here are:
- we can directly identify which scenario passes/fails
- we can avoid the
shouldTrackbranching in the test all together. IMHO unit tests should almost never contain conditional assertions as this usually indicates testing different expected outcomes in one test. (Sometimes there's an argument for this in case there'd be a lot of code to repeat but in this case I think we can avoid repetition pretty well)
happy to let you make the call on this, so feel free to disregard. Also maybe I missed a reason why the current form makes more sense.
| }); | ||
|
|
||
| it('should finish root span on timer after component spans end', () => { | ||
| // todo/fixme: This root span is only finished if trackComponents is true --> it should probably be always finished |
There was a problem hiding this comment.
hmm this looks like a bug then? according to this code trackComponents shouldn't influence the root component span. Feel free to fix directly or follow up with another PR. Whatever you prefer :) (let me know if I should fix it in case you're booked, also fine!)
This PR fixes a bug discovered in #16486 (comment) where the root component span would not end correctly if `trackComponents` was `false`. Also added a comment to explain the purpose of the root component `ui.vue.render` span. We might want to look into renaming or removing this span in the future but for now, let's fix the behaviour.
There were no tests yet for the Vue tracing mixins. To be more comfortable when continuing working on it, it's better to have some :D