Fix non-standard data type tooltips and add tests #1991
Conversation
rogermparent
changed the title
|
Consider: #1814 (this may already be catered for but just popped into my head when watching the video) |
| export type Value = | ||
| | string | ||
| | number | ||
| | boolean | ||
| | null | ||
| | number[] | ||
| | string[] | ||
| | boolean[] | ||
| type SingleValue = string | number | boolean | null | ||
| export type Value = SingleValue | SingleValue[] |
There was a problem hiding this comment.
This allows the Value type to hold mixed but not nested lists.
| TableData | ||
| } from '../../../experiments/webview/contract' | ||
|
|
||
| export const dataTypesOutput: ExperimentsOutput = { |
There was a problem hiding this comment.
We'll probably add more types here, I just included what has been asked for and we can work from there- I haven't tried nested lists or objects, and null breaks the table when given but it doesn't occur in JSON.
| it('should show the expected tooltip for all data types', () => { | ||
| const expectTooltipValue: (args: { | ||
| cellLabel: string | ||
| expectedTooltipResult: string | ||
| }) => void = ({ cellLabel, expectedTooltipResult }) => { | ||
| const testParamCell = screen.getByText(cellLabel) | ||
| expect(testParamCell).toBeInTheDocument() | ||
|
|
||
| fireEvent.mouseEnter(testParamCell, { bubbles: true }) | ||
|
|
||
| jest.advanceTimersByTime(CELL_TOOLTIP_DELAY) | ||
| const tooltip = screen.queryByRole('tooltip') | ||
| expect(tooltip).toHaveTextContent(expectedTooltipResult) | ||
|
|
||
| fireEvent.mouseLeave(testParamCell, { bubbles: true }) | ||
|
|
||
| jest.advanceTimersByTime(CELL_TOOLTIP_DELAY) | ||
| expect(screen.queryByRole('tooltip')).not.toBeInTheDocument() | ||
| } | ||
|
|
||
| render(<App />) | ||
| fireEvent( | ||
| window, | ||
| new MessageEvent('message', { | ||
| data: { | ||
| data: testData, | ||
| data: dataTypesTableData, | ||
| type: MessageToWebviewType.SET_DATA | ||
| } | ||
| }) | ||
| ) | ||
|
|
||
| const testMetricCell = screen.getByText('1.9293') | ||
| fireEvent.mouseEnter(testMetricCell, { bubbles: true }) | ||
|
|
||
| jest.advanceTimersByTime(CELL_TOOLTIP_DELAY) | ||
| const tooltip = screen.getByRole('tooltip') | ||
| expect(tooltip).toBeInTheDocument() | ||
| expect(tooltip).toHaveTextContent(String(testMetricNumberValue)) | ||
| expectTooltipValue({ | ||
| cellLabel: '1.9293', | ||
| expectedTooltipResult: '1.9293040037155151' | ||
| }) | ||
| expectTooltipValue({ | ||
| cellLabel: 'true', | ||
| expectedTooltipResult: 'true' | ||
| }) | ||
| expectTooltipValue({ | ||
| cellLabel: 'false', | ||
| expectedTooltipResult: 'false' | ||
| }) | ||
| expectTooltipValue({ | ||
| cellLabel: '[true, false, string, 2]', | ||
| expectedTooltipResult: '[true, false, string, 2]' | ||
| }) |
There was a problem hiding this comment.
Multiple test titles would have more clean messages, but I don't have the heart to re-render the same fixture 3+ more times to do it.
| className={tooltipStyles.padded} | ||
| content={<CellTooltip cell={cell} />} | ||
| content={<CellTooltip stringValue={stringValue} />} |
There was a problem hiding this comment.
Padding style moved into the base element
This should make writing play functions easier, since we can use the full testing-library query suite
|
Looks like the breakage in Storybook is from this PR: #2005 (comment) |
|
Did you reshuffle the PRs and not update the description titles? #2005-> this is the wrong way round, right? |
|
I just messed up the direction of arrows, I usually just read the ticket numbers left-to-right and don't really look at the arrows enough to even remember the usual direction apparently. |
|
Code Climate has analyzed commit 359008a and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 96.6% (0.0% change). View more on Code Climate. |
2 participants
#2005 <- this
fixes #1954
This PR adds a new test fixture whose purpose is to contain all data types that we support, so we can ensure they work correctly.
There is also a unit tests that loads this fixture and tests each cell's tooltip content. There is also a Storybook using the fixture so it can be seen in a real browser.
The original purpose of this PR was to fix a regression I made within #1967 while adding
stopPropagationto the tooltips where I forgot to re-add theStringconversion.Also refactored other parts of the tooltip to be slightly more efficient.
tooltip-data-types-fix-demo.mp4