Add tests for trace view data frames #133
Conversation
|
Levitate is-compatible report: 🔍 Resolving @grafana/data@latest... 🔬 Checking compatibility between ./src/module.ts and @grafana/data@9.4.7... 🔬 Checking compatibility between ./src/module.ts and @grafana/ui@9.4.7... 🔬 Checking compatibility between ./src/module.ts and @grafana/runtime@9.4.7... 🔬 Checking compatibility between ./src/module.ts and @grafana/e2e-selectors@9.4.7... ✔️ ./src/module.ts appears to be compatible with @grafana/data,@grafana/ui,@grafana/runtime,@grafana/e2e-selectors |
|
Backend code coverage report for PR #133 |
|
Frontend code coverage report for PR #133
|
There was a problem hiding this comment.
Cool stuff Ida!
This is a bit nitpicky so please take it or leave it: my general preference when it comes to this sort of thing is to only test the top level exported function. My reasoning with that is that it might be that we someday change up something in transformTraceResponse that causes a test to fail, but that doesn't necessarily prove something is broken. What we want is to test that we can take a trace and turn it into a dataframe, and the sort of particulars of how we go about that can be changed pretty frequently but hopefully the tests that prove we can handle creating a dataframe do not change.
Yeah, that makes sense! I initially went with that approach, but though about how it would be more tricky to debug failing tests if we don't know which of the several functions was responsible for it failing. I'll think about how to have the best of both worlds and rewrite :) |
|
Rad up to you! No pressure to rewrite more just sharing a thought! But if you feel inspired to go for it! |
| @@ -0,0 +1,156 @@ | |||
| // cSpell:disable | |||
There was a problem hiding this comment.
oh neat I didn't know you could do this I always just add weird words to the list 😆 good to know
| new Date('2023-04-11T11:14:30.717151Z').getTime(), | ||
| ]) | ||
| ); | ||
| // cSpell:disable |
There was a problem hiding this comment.
maybe better to add the list of weird words rather than not use cspell? idk I guess either way haha
There was a problem hiding this comment.
I disabled this block cause these words are probably just a one-time thing specific to this test and they most likely won't be repeated anywhere else
src/traces/formatTraces.ts
Outdated
| @@ -129,27 +130,27 @@ export const createTraceDataFrame = (targets, response): DataQueryResponse => { | |||
| return { data: dataFrames, key: targets[0].refId }; | |||
| }; | |||
|
|
|||
| function transformTraceResponse(spanList: OpenSearchSpan[]): TraceSpanRow[] { | |||
| export function transformTraceResponse(spanList: OpenSearchSpan[]): TraceSpanRow[] { | |||
There was a problem hiding this comment.
I don't think you need to export anymore right?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: