remove snapshots testing assertions #530
Conversation
There was a problem hiding this comment.
Hey @cescoferraro GJ! I left some comments and concerns. Let me know what you think
| cursor: pointer; | ||
|
|
||
| ${({isCollapsed}) => | ||
| isCollapsed | ||
| ${({$isCollapsed}) => |
There was a problem hiding this comment.
What's the idea around the $? is this causing any warnings or something like that?
There was a problem hiding this comment.
There was a problem hiding this comment.
it causes warning on the console & on testing
| @@ -29,6 +29,7 @@ const Diagram: React.FC<IDiagramProps> = ({trace, selectedSpan, onSelectSpan}): | |||
| ); | |||
| }, [trace?.spans]); | |||
|
|
|||
| console.log(spanMap); | |||
| @@ -1,5 +1,5 @@ | |||
| import {capitalize} from 'lodash'; | |||
| import {useCallback} from 'react'; | |||
| import React, {useCallback} from 'react'; | |||
There was a problem hiding this comment.
I don't think we should need to explicitly import React anymore
| @@ -9,8 +9,11 @@ import TestCardActions from './TestCardActions'; | |||
|
|
|||
| interface TTestCardProps { | |||
| test: TTest; | |||
|
|
|||
There was a problem hiding this comment.
I'd like to see what prettier rules are you using, mine doesn't add the extra whitespace
| import {TTest} from '../../../types/Test.types'; | ||
| import {HTTP_METHOD} from '../../../constants/Common.constants'; | ||
|
|
||
| const spanId = '4938u43'; |
There was a problem hiding this comment.
Consider using the mock factories, you can overwrite the different values if you want
| @@ -13,6 +13,7 @@ const TraceMock: IMockFactory<TTrace, TRawTrace> = () => ({ | |||
| '1': SpanMock.raw(), | |||
| '2': SpanMock.raw(), | |||
| }, | |||
| spans: [SpanMock.raw(), SpanMock.raw()], | |||
There was a problem hiding this comment.
The raw trace doesn't have a spans array, it either comes from the flat or the tree section.
| ({key, operator = CompareOperator.EQUALS, value}) => | ||
| `${key}${OperatorService.getOperatorSymbol(operator)}${getValue(value)}` | ||
| ); | ||
| selectors.map(({key, operator = CompareOperator.EQUALS, value}) => { |
There was a problem hiding this comment.
I'm changing some of this on my next PR as I'm working on moving the assertions to the backend
cescoferraro
force-pushed
the
feat/remove_snapshots
branch
from
4bd3339
to
1b8eae4
Compare
| @@ -46,7 +49,7 @@ const getPseudoSelectorString = (pseudoSelector?: TPseudoSelector): string => { | |||
| const SelectorService = () => ({ | |||
| getSelectorString(selectorList: TSpanSelector[], pseudoSelector?: TPseudoSelector): string { | |||
| return selectorList.length | |||
| ? `span[${getFilters(selectorList).join(' ')}]${getPseudoSelectorString(pseudoSelector)}` | |||
There was a problem hiding this comment.
We need the double space to make this work, as we are doing a double space split on line 61
web/src/services/Selector.service.ts
Outdated
| selectors.map(({key, operator, value}) => `${key} ${operator} ${getValue(value)}`); | ||
| const getFilters = (selectors: TSpanSelector[]) => { | ||
| return selectors.map( | ||
| ({key, operator, value}) => `${key}${operator === 'contains' ? ` ${operator} ` : operator}${getValue(value)}` |
There was a problem hiding this comment.
We can always have the spaces between for the operator so I'm not sure that we need to check for contains directly.
There was a problem hiding this comment.
Let's roll back the changes made around the selectors and we should be good to merge!
cescoferraro
force-pushed
the
feat/remove_snapshots
branch
3 times, most recently
from
9922fb0
to
b0c35d6
Compare
cescoferraro
force-pushed
the
feat/remove_snapshots
branch
from
b0c35d6
to
d97e546
Compare
cescoferraro
force-pushed
the
feat/remove_snapshots
branch
from
d97e546
to
1b5c885
Compare
This PR removes all snapshot testing assertions and fixes most of the tests
Checklist