From 6ea433baf9f13cb21c2f36b020fd01eb3e50f2b2 Mon Sep 17 00:00:00 2001 From: rogermparent Date: Tue, 5 Jul 2022 19:38:01 -0400 Subject: [PATCH 01/22] Fix nonstandard type tooltips and refactor --- .../experiments/components/table/styles.module.scss | 4 ++++ webview/src/experiments/util/buildDynamicColumns.tsx | 12 +++++------- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/webview/src/experiments/components/table/styles.module.scss b/webview/src/experiments/components/table/styles.module.scss index a33846a00a..cb2a33ee93 100644 --- a/webview/src/experiments/components/table/styles.module.scss +++ b/webview/src/experiments/components/table/styles.module.scss @@ -663,3 +663,7 @@ $badge-size: 0.85rem; vertical-align: middle; font-size: 0.5rem; } + +.cellTooltip { + padding: 2px 6px; +} diff --git a/webview/src/experiments/util/buildDynamicColumns.tsx b/webview/src/experiments/util/buildDynamicColumns.tsx index 01882b23c5..c5572906c8 100644 --- a/webview/src/experiments/util/buildDynamicColumns.tsx +++ b/webview/src/experiments/util/buildDynamicColumns.tsx @@ -13,7 +13,6 @@ import Tooltip, { CELL_TOOLTIP_DELAY } from '../../shared/components/tooltip/Tooltip' import styles from '../components/table/styles.module.scss' -import tooltipStyles from '../../shared/components/tooltip/styles.module.scss' import { CopyButton } from '../../shared/components/copyButton/CopyButton' import { OverflowHoverTooltip } from '../components/overflowHoverTooltip/OverflowHoverTooltip' const UndefinedCell = ( @@ -23,11 +22,11 @@ const UndefinedCell = ( ) const CellTooltip: React.FC<{ - cell: Cell -}> = ({ cell }) => { - const { value } = cell + stringValue: string +}> = ({ stringValue }) => { return (
{ @@ -37,7 +36,7 @@ const CellTooltip: React.FC<{ e.stopPropagation() }} > - {value} + {stringValue}
) } @@ -57,8 +56,7 @@ const Cell: React.FC> = cell => { return ( } + content={} placement="bottom" arrow={true} delay={[CELL_TOOLTIP_DELAY, 0]} From fae24b4d6162858f2b66eb426a661f7ee5ac2a53 Mon Sep 17 00:00:00 2001 From: rogermparent Date: Tue, 5 Jul 2022 21:26:07 -0400 Subject: [PATCH 02/22] Refactor stopPropagation event on tooltips --- webview/src/experiments/util/buildDynamicColumns.tsx | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/webview/src/experiments/util/buildDynamicColumns.tsx b/webview/src/experiments/util/buildDynamicColumns.tsx index c5572906c8..4207665c9e 100644 --- a/webview/src/experiments/util/buildDynamicColumns.tsx +++ b/webview/src/experiments/util/buildDynamicColumns.tsx @@ -1,4 +1,4 @@ -import React from 'react' +import React, { SyntheticEvent } from 'react' import get from 'lodash/get' import { Column as TableColumn, @@ -24,17 +24,14 @@ const UndefinedCell = ( const CellTooltip: React.FC<{ stringValue: string }> = ({ stringValue }) => { + const stopPropagation = (e: SyntheticEvent) => e.stopPropagation() return (
{ - e.stopPropagation() - }} - onKeyDown={e => { - e.stopPropagation() - }} + onClick={stopPropagation} + onKeyDown={stopPropagation} > {stringValue}
From 714880baf04f27c82b91b8edb4a404706e7120f1 Mon Sep 17 00:00:00 2001 From: rogermparent Date: Wed, 6 Jul 2022 00:40:09 -0400 Subject: [PATCH 03/22] Add generic tooltip test builder and use it for multiple data types --- .../src/experiments/components/App.test.tsx | 250 ++++++++++-------- 1 file changed, 146 insertions(+), 104 deletions(-) diff --git a/webview/src/experiments/components/App.test.tsx b/webview/src/experiments/components/App.test.tsx index 2077b57775..f4e2bc762f 100644 --- a/webview/src/experiments/components/App.test.tsx +++ b/webview/src/experiments/components/App.test.tsx @@ -1,7 +1,7 @@ /** * @jest-environment jsdom */ -/* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "expectHeaders"] }] */ +/* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "expectHeaders", "expectTooltipValue"] }] */ import React from 'react' import { cleanup, @@ -60,6 +60,85 @@ afterEach(() => { cleanup() }) +const sourceFilenames: Record = { + [ColumnType.PARAMS]: 'params.yaml', + [ColumnType.METRICS]: 'metrics.json', + [ColumnType.DEPS]: 'data' +} + +const buildTestColumn = ({ + name, + type: columnType = ColumnType.PARAMS, + sourceFilename = sourceFilenames[columnType], + ...rest +}: Partial & { + name: string + columnType?: ColumnType + sourceFilename?: string +}): Column => ({ + hasChildren: false, + label: name, + parentPath: buildMetricOrParamPath(columnType, sourceFilename), + path: buildMetricOrParamPath(columnType, sourceFilename, name), + pathArray: [columnType, sourceFilename, name], + type: columnType, + ...rest +}) + +const buildTestData: ( + args: { + value: T + name?: string + otherValue?: T + types?: string[] + } & Partial +) => TableData = ({ + value, + name = 'test-param', + types = [typeof value], + ...rest +}) => { + return { + ...tableDataFixture, + columns: [ + { + hasChildren: true, + label: 'summary.json', + parentPath: buildMetricOrParamPath(ColumnType.METRICS), + path: buildMetricOrParamPath(ColumnType.METRICS, 'summary.json'), + type: ColumnType.METRICS + }, + { + hasChildren: true, + label: 'params.yaml', + parentPath: ColumnType.PARAMS, + path: buildMetricOrParamPath(ColumnType.PARAMS, 'params.yaml'), + type: ColumnType.PARAMS + }, + buildTestColumn({ + name, + types, + ...rest + }) + ], + rows: [ + { + id: 'workspace', + label: 'workspace', + params: { + 'params.yaml': { + [name]: value + } + } + }, + { + id: 'main', + label: 'main' + } + ] + } +} + describe('App', () => { describe('Sorting Classes', () => { const renderTableWithPlaceholder = () => { @@ -456,6 +535,39 @@ describe('App', () => { }) describe('Tooltips', () => { + const expectTooltipValue: ( + args: { + value: T + cellLabel: string + expectedTooltipResult: string + } & Partial + ) => void = ({ value, cellLabel, expectedTooltipResult, ...rest }) => { + render() + fireEvent( + window, + new MessageEvent('message', { + data: { + data: buildTestData({ + maxStringLength: 5, + value, + ...rest + }), + type: MessageToWebviewType.SET_DATA + } + }) + ) + + const testCells = screen.getAllByRole('cell') + const testCell = within(testCells[3]).getAllByText(/.+/)?.[0] + expect(testCell).toHaveTextContent(cellLabel) + fireEvent.mouseEnter(testCell, { bubbles: true }) + + jest.advanceTimersByTime(CELL_TOOLTIP_DELAY) + const tooltip = screen.getByRole('tooltip') + expect(tooltip).toBeInTheDocument() + expect(tooltip).toHaveTextContent(expectedTooltipResult) + } + beforeAll(() => { jest.useFakeTimers() }) @@ -463,93 +575,13 @@ describe('App', () => { jest.useRealTimers() }) - const testParamName = 'test_param_with_long_name' - const testParamPath = buildMetricOrParamPath( - ColumnType.PARAMS, - 'params.yaml', - testParamName - ) - const testParamStringValue = 'Test Value' - const testMetricNumberValue = 1.9293040037155151 - - const testData = { - ...tableDataFixture, - columns: [ - { - hasChildren: true, - label: 'summary.json', - parentPath: buildMetricOrParamPath(ColumnType.METRICS), - path: buildMetricOrParamPath(ColumnType.METRICS, 'summary.json'), - type: ColumnType.METRICS - }, - { - hasChildren: false, - label: 'loss', - maxNumber: testMetricNumberValue, - maxStringLength: 18, - minNumber: testMetricNumberValue, - parentPath: buildMetricOrParamPath( - ColumnType.METRICS, - 'summary.json' - ), - path: buildMetricOrParamPath( - ColumnType.METRICS, - 'summary.json', - 'loss' - ), - pathArray: [ColumnType.METRICS, 'summary.json', 'loss'], - type: ColumnType.METRICS, - types: ['number'] - }, - { - hasChildren: true, - label: 'params.yaml', - parentPath: ColumnType.PARAMS, - path: buildMetricOrParamPath(ColumnType.PARAMS, 'params.yaml'), - type: ColumnType.PARAMS - }, - { - hasChildren: false, - label: testParamName, - maxStringLength: 10, - parentPath: buildMetricOrParamPath(ColumnType.PARAMS, 'params.yaml'), - path: testParamPath, - pathArray: [ColumnType.PARAMS, 'params.yaml', testParamName], - type: ColumnType.PARAMS, - types: ['string'] - } - ], - rows: [ - { - id: 'workspace', - label: 'workspace', - metrics: { - 'summary.json': { - loss: testMetricNumberValue - } - }, - params: { - 'params.yaml': { - [testParamName]: testParamStringValue - } - } - }, - { - id: 'main', - label: 'main', - metrics: { - 'summary.json': { - loss: testMetricNumberValue + 1 - } - }, - params: { - 'params.yaml': { - [testParamName]: 'Other Value' - } - } - } - ] - } + const testParamName = 'test-param' + const testParamStringValue = 'Test String' + const testData = buildTestData({ + maxStringLength: 10, + name: testParamName, + value: testParamStringValue + }) it('should show and hide a tooltip on mouseEnter and mouseLeave of a header', () => { mockedUseIsFullyContained.mockReturnValue(false) @@ -648,24 +680,34 @@ describe('App', () => { }) it('should show a tooltip with the full number on number cells', () => { - render() - fireEvent( - window, - new MessageEvent('message', { - data: { - data: testData, - type: MessageToWebviewType.SET_DATA - } - }) - ) + const testNumber = 1.9293040037155151 + expectTooltipValue({ + cellLabel: '1.9293', + expectedTooltipResult: String(testNumber), + value: testNumber + }) + }) - const testMetricCell = screen.getByText('1.9293') - fireEvent.mouseEnter(testMetricCell, { bubbles: true }) + it('should show a tooltip with false and true', () => { + expectTooltipValue({ + cellLabel: 'true', + expectedTooltipResult: 'true', + value: true + }) + expectTooltipValue({ + cellLabel: 'false', + expectedTooltipResult: 'false', + value: false + }) + }) - jest.advanceTimersByTime(CELL_TOOLTIP_DELAY) - const tooltip = screen.getByRole('tooltip') - expect(tooltip).toBeInTheDocument() - expect(tooltip).toHaveTextContent(String(testMetricNumberValue)) + it('should show a tooltip with a stringified array', () => { + const stringifiedArray = '[true, false, string, 2]' + expectTooltipValue({ + cellLabel: stringifiedArray, + expectedTooltipResult: stringifiedArray, + value: [true, false, 'string', 2] + }) }) }) From 4bfbbd370483617808920d5a80cff369b26ea7a6 Mon Sep 17 00:00:00 2001 From: rogermparent Date: Wed, 6 Jul 2022 01:10:55 -0400 Subject: [PATCH 04/22] Use more static values in tests --- webview/src/experiments/components/App.test.tsx | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/webview/src/experiments/components/App.test.tsx b/webview/src/experiments/components/App.test.tsx index f4e2bc762f..9957f2055a 100644 --- a/webview/src/experiments/components/App.test.tsx +++ b/webview/src/experiments/components/App.test.tsx @@ -680,20 +680,22 @@ describe('App', () => { }) it('should show a tooltip with the full number on number cells', () => { - const testNumber = 1.9293040037155151 expectTooltipValue({ cellLabel: '1.9293', - expectedTooltipResult: String(testNumber), - value: testNumber + expectedTooltipResult: '1.9293040037155151', + value: 1.9293040037155151 }) }) - it('should show a tooltip with false and true', () => { + it('should show the right tooltip for true', () => { expectTooltipValue({ cellLabel: 'true', expectedTooltipResult: 'true', value: true }) + }) + + it('should show the right tooltip for false', () => { expectTooltipValue({ cellLabel: 'false', expectedTooltipResult: 'false', @@ -702,10 +704,9 @@ describe('App', () => { }) it('should show a tooltip with a stringified array', () => { - const stringifiedArray = '[true, false, string, 2]' expectTooltipValue({ - cellLabel: stringifiedArray, - expectedTooltipResult: stringifiedArray, + cellLabel: '[true, false, string, 2]', + expectedTooltipResult: '[true, false, string, 2]', value: [true, false, 'string', 2] }) }) From a71be71bc1976c21919bc612c8ffdf9935ab8c2d Mon Sep 17 00:00:00 2001 From: rogermparent Date: Wed, 6 Jul 2022 13:04:00 -0400 Subject: [PATCH 05/22] Add data types fixture --- extension/src/cli/reader.ts | 10 +- .../src/experiments/columns/model.test.ts | 15 +- extension/src/experiments/model/index.test.ts | 14 +- .../src/test/fixtures/expShow/dataTypes.ts | 193 ++++++++++++++++++ webview/src/stories/Table.stories.tsx | 4 + 5 files changed, 224 insertions(+), 12 deletions(-) create mode 100644 extension/src/test/fixtures/expShow/dataTypes.ts diff --git a/extension/src/cli/reader.ts b/extension/src/cli/reader.ts index b38618d45f..7c2f6ec166 100644 --- a/extension/src/cli/reader.ts +++ b/extension/src/cli/reader.ts @@ -49,14 +49,8 @@ export type StatusesOrAlwaysChanged = StageOrFileStatuses | 'always changed' export type StatusOutput = Record -export type Value = - | string - | number - | boolean - | null - | number[] - | string[] - | boolean[] +type SingleValue = string | number | boolean | null +export type Value = SingleValue | SingleValue[] export interface ValueTreeOrError { data?: ValueTree diff --git a/extension/src/experiments/columns/model.test.ts b/extension/src/experiments/columns/model.test.ts index f85ccdc959..3caa77d951 100644 --- a/extension/src/experiments/columns/model.test.ts +++ b/extension/src/experiments/columns/model.test.ts @@ -10,21 +10,32 @@ import { deeplyNestedOutput, columns as deeplyNestedColumns } from '../../test/fixtures/expShow/deeplyNested' +import { + dataTypesOutput, + columns as dataTypesColumns +} from '../../test/fixtures/expShow/dataTypes' describe('ColumnsModel', () => { const exampleDvcRoot = 'test' - it('should return columns that equal the columns fixture when given the output fixture', async () => { + it('should return columns that equal the default columns fixture when given the default output fixture', async () => { const model = new ColumnsModel('', buildMockMemento()) await model.transformAndSet(outputFixture) expect(model.getSelected()).toStrictEqual(columnsFixture) }) - it('should return data that equal the deeply nested output fixture', async () => { + it('should return expected columns that equal the deeply nested output fixture', async () => { const model = new ColumnsModel('', buildMockMemento()) await model.transformAndSet(deeplyNestedOutput) expect(model.getSelected()).toStrictEqual(deeplyNestedColumns) }) + + it('should return expected columns that equal the data types output fixture', async () => { + const model = new ColumnsModel('', buildMockMemento()) + await model.transformAndSet(dataTypesOutput) + expect(model.getSelected()).toStrictEqual(dataTypesColumns) + }) + describe('persistence', () => { const paramsDotYamlPath = buildMetricOrParamPath( ColumnType.PARAMS, diff --git a/extension/src/experiments/model/index.test.ts b/extension/src/experiments/model/index.test.ts index 04edc91417..23c9d5a021 100644 --- a/extension/src/experiments/model/index.test.ts +++ b/extension/src/experiments/model/index.test.ts @@ -12,6 +12,10 @@ import { buildMockMemento } from '../../test/util' import { buildMetricOrParamPath } from '../columns/paths' import { Experiment, ColumnType } from '../webview/contract' import { definedAndNonEmpty } from '../../util/array' +import { + dataTypesOutput, + rows as dataTypesRows +} from '../../test/fixtures/expShow/dataTypes' jest.mock('vscode') @@ -64,18 +68,24 @@ describe('ExperimentsModel', () => { return { data } } - it('should return rows that equal the rows fixture when given the output fixture', () => { + it('should return expected rows when given the output fixture', () => { const model = new ExperimentsModel('', buildMockMemento()) model.transformAndSet(outputFixture) expect(model.getRowData()).toStrictEqual(rowsFixture) }) - it('should return data that equal the deeply nested output fixture', () => { + it('should return expected rows the deeply nested output fixture', () => { const model = new ExperimentsModel('', buildMockMemento()) model.transformAndSet(deeplyNestedOutput) expect(model.getRowData()).toStrictEqual(deeplyNestedRows) }) + it('should return expected rows the data types output fixture', () => { + const model = new ExperimentsModel('', buildMockMemento()) + model.transformAndSet(dataTypesOutput) + expect(model.getRowData()).toStrictEqual(dataTypesRows) + }) + it('should continue to apply filters to new data if selection mode is set to use filters', () => { const testPath = buildMetricOrParamPath( ColumnType.PARAMS, diff --git a/extension/src/test/fixtures/expShow/dataTypes.ts b/extension/src/test/fixtures/expShow/dataTypes.ts new file mode 100644 index 0000000000..40ae19e7d5 --- /dev/null +++ b/extension/src/test/fixtures/expShow/dataTypes.ts @@ -0,0 +1,193 @@ +import { ExperimentsOutput } from '../../../cli/reader' +import { + Column, + ColumnType, + Row, + TableData +} from '../../../experiments/webview/contract' + +export const dataTypesOutput: ExperimentsOutput = { + workspace: { + baseline: { + data: { + timestamp: null, + params: { + 'params.yaml': { + data: { + true: true, + false: false, + zero: 0, + negative: -123, + float: 0.123, + string: 'string', + emptyString: '', + array: [true, false, 2, 'string'] + } + } + }, + queued: false, + running: false, + executor: null + } + } + }, + '53c3851f46955fa3e2b8f6e1c52999acc8c9ea77': { + baseline: { + data: { + timestamp: '2020-11-21T19:58:22', + queued: false, + running: false, + executor: null, + name: 'main' + } + } + } +} + +export const columns: Column[] = [ + { + hasChildren: true, + label: 'params.yaml', + parentPath: ColumnType.PARAMS, + path: 'params:params.yaml', + type: ColumnType.PARAMS + }, + { + hasChildren: false, + label: 'true', + maxStringLength: 4, + parentPath: 'params:params.yaml', + path: 'params:params.yaml:true', + pathArray: [ColumnType.PARAMS, 'params.yaml', 'true'], + type: ColumnType.PARAMS, + types: ['boolean'] + }, + { + hasChildren: false, + label: 'false', + maxStringLength: 5, + parentPath: 'params:params.yaml', + path: 'params:params.yaml:false', + pathArray: [ColumnType.PARAMS, 'params.yaml', 'false'], + type: ColumnType.PARAMS, + types: ['boolean'] + }, + { + hasChildren: false, + label: 'zero', + maxNumber: 0, + maxStringLength: 1, + minNumber: 0, + parentPath: 'params:params.yaml', + path: 'params:params.yaml:zero', + pathArray: [ColumnType.PARAMS, 'params.yaml', 'zero'], + type: ColumnType.PARAMS, + types: ['number'] + }, + { + hasChildren: false, + label: 'negative', + maxNumber: -123, + maxStringLength: 4, + minNumber: -123, + parentPath: 'params:params.yaml', + path: 'params:params.yaml:negative', + pathArray: [ColumnType.PARAMS, 'params.yaml', 'negative'], + type: ColumnType.PARAMS, + types: ['number'] + }, + { + hasChildren: false, + label: 'float', + maxNumber: 0.123, + maxStringLength: 5, + minNumber: 0.123, + parentPath: 'params:params.yaml', + path: 'params:params.yaml:float', + pathArray: [ColumnType.PARAMS, 'params.yaml', 'float'], + type: ColumnType.PARAMS, + types: ['number'] + }, + { + hasChildren: false, + label: 'string', + maxStringLength: 6, + parentPath: 'params:params.yaml', + path: 'params:params.yaml:string', + pathArray: [ColumnType.PARAMS, 'params.yaml', 'string'], + type: ColumnType.PARAMS, + types: ['string'] + }, + { + hasChildren: false, + label: 'emptyString', + maxStringLength: 0, + parentPath: 'params:params.yaml', + path: 'params:params.yaml:emptyString', + pathArray: [ColumnType.PARAMS, 'params.yaml', 'emptyString'], + type: ColumnType.PARAMS, + types: ['string'] + }, + { + hasChildren: false, + label: 'array', + maxStringLength: 19, + parentPath: 'params:params.yaml', + path: 'params:params.yaml:array', + pathArray: [ColumnType.PARAMS, 'params.yaml', 'array'], + type: ColumnType.PARAMS, + types: ['array'] + } +] +export const rows: Row[] = [ + { + displayColor: '#945dd6', + executor: null, + id: 'workspace', + label: 'workspace', + params: { + 'params.yaml': { + array: [true, false, 2, 'string'], + emptyString: '', + false: false, + float: 0.123, + negative: -123, + string: 'string', + true: true, + zero: 0 + } + }, + queued: false, + running: false, + selected: true, + timestamp: null + }, + { + displayColor: '#13adc7', + executor: null, + id: 'main', + label: 'main', + name: 'main', + queued: false, + running: false, + selected: true, + sha: '53c3851f46955fa3e2b8f6e1c52999acc8c9ea77', + timestamp: '2020-11-21T19:58:22' + } +] + +export const dataTypesTableData: TableData = { + changes: [], + columnOrder: [], + columnWidths: {}, + filteredCounts: { experiments: 0, checkpoints: 0 }, + filters: [], + hasCheckpoints: false, + hasRunningExperiment: false, + sorts: [], + columns, + hasColumns: true, + rows +} + +export default dataTypesTableData diff --git a/webview/src/stories/Table.stories.tsx b/webview/src/stories/Table.stories.tsx index 5318a5a88d..c53b595031 100644 --- a/webview/src/stories/Table.stories.tsx +++ b/webview/src/stories/Table.stories.tsx @@ -5,6 +5,7 @@ import columnsFixture from 'dvc/src/test/fixtures/expShow/columns' import { TableData } from 'dvc/src/experiments/webview/contract' import workspaceChangesFixture from 'dvc/src/test/fixtures/expShow/workspaceChanges' import deeplyNestedTableData from 'dvc/src/test/fixtures/expShow/deeplyNested' +import { dataTypesTableData } from 'dvc/src/test/fixtures/expShow/dataTypes' import Experiments from '../experiments/components/Experiments' import './test-vscode-styles.scss' @@ -79,6 +80,9 @@ WithNoRunningExperiments.args = { } } +export const WithAllDataTypes = Template.bind({}) +WithAllDataTypes.args = { tableData: dataTypesTableData } + export const WithDeeplyNestedHeaders = Template.bind({}) WithDeeplyNestedHeaders.args = { tableData: deeplyNestedTableData } From 30706efd2de523984c49c32af0c17131d031f7cb Mon Sep 17 00:00:00 2001 From: rogermparent Date: Wed, 6 Jul 2022 15:22:36 -0400 Subject: [PATCH 06/22] Add failing tests using new fixture --- .../src/test/fixtures/expShow/dataTypes.ts | 14 +-- .../src/experiments/components/App.test.tsx | 97 ++++++++----------- 2 files changed, 49 insertions(+), 62 deletions(-) diff --git a/extension/src/test/fixtures/expShow/dataTypes.ts b/extension/src/test/fixtures/expShow/dataTypes.ts index 40ae19e7d5..65d3ddfffd 100644 --- a/extension/src/test/fixtures/expShow/dataTypes.ts +++ b/extension/src/test/fixtures/expShow/dataTypes.ts @@ -18,10 +18,10 @@ export const dataTypesOutput: ExperimentsOutput = { false: false, zero: 0, negative: -123, - float: 0.123, + float: 1.9293040037155151, string: 'string', emptyString: '', - array: [true, false, 2, 'string'] + array: [true, false, 'string', 2] } } }, @@ -99,9 +99,9 @@ export const columns: Column[] = [ { hasChildren: false, label: 'float', - maxNumber: 0.123, - maxStringLength: 5, - minNumber: 0.123, + maxNumber: 1.9293040037155151, + maxStringLength: 18, + minNumber: 1.9293040037155151, parentPath: 'params:params.yaml', path: 'params:params.yaml:float', pathArray: [ColumnType.PARAMS, 'params.yaml', 'float'], @@ -147,10 +147,10 @@ export const rows: Row[] = [ label: 'workspace', params: { 'params.yaml': { - array: [true, false, 2, 'string'], + array: [true, false, 'string', 2], emptyString: '', false: false, - float: 0.123, + float: 1.9293040037155151, negative: -123, string: 'string', true: true, diff --git a/webview/src/experiments/components/App.test.tsx b/webview/src/experiments/components/App.test.tsx index 9957f2055a..e742a619ae 100644 --- a/webview/src/experiments/components/App.test.tsx +++ b/webview/src/experiments/components/App.test.tsx @@ -25,6 +25,7 @@ import { TableData } from 'dvc/src/experiments/webview/contract' import { buildMetricOrParamPath } from 'dvc/src/experiments/columns/paths' +import { dataTypesTableData } from 'dvc/src/test/fixtures/expShow/dataTypes' import { App } from './App' import { useIsFullyContained } from './overflowHoverTooltip/useIsFullyContained' import styles from './table/styles.module.scss' @@ -535,39 +536,6 @@ describe('App', () => { }) describe('Tooltips', () => { - const expectTooltipValue: ( - args: { - value: T - cellLabel: string - expectedTooltipResult: string - } & Partial - ) => void = ({ value, cellLabel, expectedTooltipResult, ...rest }) => { - render() - fireEvent( - window, - new MessageEvent('message', { - data: { - data: buildTestData({ - maxStringLength: 5, - value, - ...rest - }), - type: MessageToWebviewType.SET_DATA - } - }) - ) - - const testCells = screen.getAllByRole('cell') - const testCell = within(testCells[3]).getAllByText(/.+/)?.[0] - expect(testCell).toHaveTextContent(cellLabel) - fireEvent.mouseEnter(testCell, { bubbles: true }) - - jest.advanceTimersByTime(CELL_TOOLTIP_DELAY) - const tooltip = screen.getByRole('tooltip') - expect(tooltip).toBeInTheDocument() - expect(tooltip).toHaveTextContent(expectedTooltipResult) - } - beforeAll(() => { jest.useFakeTimers() }) @@ -679,36 +647,55 @@ describe('App', () => { jest.mocked(window.requestAnimationFrame).mockRestore() }) - it('should show a tooltip with the full number on number cells', () => { - expectTooltipValue({ + it('should show the expected tooltip for all values in the data types fixture', async () => { + jest + .spyOn(window, 'requestAnimationFrame') + .mockImplementation(cb => window.setTimeout(cb, 1)) + const expectTooltipValue: (args: { + cellLabel: string + expectedTooltipResult: string + }) => Promise = async ({ cellLabel, expectedTooltipResult }) => { + const testCell = screen.getAllByText(cellLabel)?.[0] + fireEvent.mouseEnter(testCell, { bubbles: true }) + jest.advanceTimersByTime(CELL_TOOLTIP_DELAY) + const tooltip = await screen.findByRole('tooltip') + expect(tooltip).toHaveTextContent(expectedTooltipResult) + jest.useRealTimers() + await new Promise(resolve => setTimeout(resolve, 1)) + fireEvent.mouseLeave(testCell, { bubbles: true }) + await new Promise(resolve => setTimeout(resolve, 1)) + jest.useFakeTimers() + expect(screen.queryByRole('tooltip')).not.toBeInTheDocument() + } + + render() + fireEvent( + window, + new MessageEvent('message', { + data: { + data: dataTypesTableData, + type: MessageToWebviewType.SET_DATA + } + }) + ) + + await expectTooltipValue({ cellLabel: '1.9293', - expectedTooltipResult: '1.9293040037155151', - value: 1.9293040037155151 + expectedTooltipResult: '1.9293040037155151' }) - }) - - it('should show the right tooltip for true', () => { - expectTooltipValue({ + await expectTooltipValue({ cellLabel: 'true', - expectedTooltipResult: 'true', - value: true + expectedTooltipResult: 'true' }) - }) - - it('should show the right tooltip for false', () => { - expectTooltipValue({ + await expectTooltipValue({ cellLabel: 'false', - expectedTooltipResult: 'false', - value: false + expectedTooltipResult: 'false' }) - }) - - it('should show a tooltip with a stringified array', () => { - expectTooltipValue({ + await expectTooltipValue({ cellLabel: '[true, false, string, 2]', - expectedTooltipResult: '[true, false, string, 2]', - value: [true, false, 'string', 2] + expectedTooltipResult: '[true, false, string, 2]' }) + jest.mocked(window.requestAnimationFrame).mockRestore() }) }) From c22f48642989ae8e5cb7bb0bd18c264190a0e3a0 Mon Sep 17 00:00:00 2001 From: rogermparent Date: Fri, 8 Jul 2022 00:16:49 -0400 Subject: [PATCH 07/22] Rename columns and fix tooltip tests --- .../src/test/fixtures/expShow/dataTypes.ts | 20 +++--- .../src/experiments/components/App.test.tsx | 65 +++++++++---------- 2 files changed, 39 insertions(+), 46 deletions(-) diff --git a/extension/src/test/fixtures/expShow/dataTypes.ts b/extension/src/test/fixtures/expShow/dataTypes.ts index 65d3ddfffd..e58072761b 100644 --- a/extension/src/test/fixtures/expShow/dataTypes.ts +++ b/extension/src/test/fixtures/expShow/dataTypes.ts @@ -14,8 +14,8 @@ export const dataTypesOutput: ExperimentsOutput = { params: { 'params.yaml': { data: { - true: true, - false: false, + bool1: true, + bool2: false, zero: 0, negative: -123, float: 1.9293040037155151, @@ -54,21 +54,21 @@ export const columns: Column[] = [ }, { hasChildren: false, - label: 'true', + label: 'bool1', maxStringLength: 4, parentPath: 'params:params.yaml', - path: 'params:params.yaml:true', - pathArray: [ColumnType.PARAMS, 'params.yaml', 'true'], + path: 'params:params.yaml:bool1', + pathArray: [ColumnType.PARAMS, 'params.yaml', 'bool1'], type: ColumnType.PARAMS, types: ['boolean'] }, { hasChildren: false, - label: 'false', + label: 'bool2', maxStringLength: 5, parentPath: 'params:params.yaml', - path: 'params:params.yaml:false', - pathArray: [ColumnType.PARAMS, 'params.yaml', 'false'], + path: 'params:params.yaml:bool2', + pathArray: [ColumnType.PARAMS, 'params.yaml', 'bool2'], type: ColumnType.PARAMS, types: ['boolean'] }, @@ -149,11 +149,11 @@ export const rows: Row[] = [ 'params.yaml': { array: [true, false, 'string', 2], emptyString: '', - false: false, + bool2: false, float: 1.9293040037155151, negative: -123, string: 'string', - true: true, + bool1: true, zero: 0 } }, diff --git a/webview/src/experiments/components/App.test.tsx b/webview/src/experiments/components/App.test.tsx index e742a619ae..091c0e0aa5 100644 --- a/webview/src/experiments/components/App.test.tsx +++ b/webview/src/experiments/components/App.test.tsx @@ -155,7 +155,7 @@ describe('App', () => { ) } - it('should apply the sortingHeaderCellAsc class to only a top level placeholder', () => { + it('should apply the sortingHeaderCellAsc class to) only a top level placeholder', () => { renderTableWithPlaceholder() const topPlaceholder = screen.getByTestId( @@ -544,11 +544,11 @@ describe('App', () => { }) const testParamName = 'test-param' - const testParamStringValue = 'Test String' + const cellLabel = 'Test String' const testData = buildTestData({ maxStringLength: 10, name: testParamName, - value: testParamStringValue + value: cellLabel }) it('should show and hide a tooltip on mouseEnter and mouseLeave of a header', () => { @@ -613,9 +613,6 @@ describe('App', () => { }) it('should show and hide a tooltip on mouseEnter and mouseLeave of a cell', () => { - jest - .spyOn(window, 'requestAnimationFrame') - .mockImplementation(cb => window.setTimeout(cb, 1)) render() fireEvent( window, @@ -627,7 +624,7 @@ describe('App', () => { }) ) - const testParamCell = screen.getByText(testParamStringValue) + const testParamCell = screen.getByText(cellLabel) fireEvent.mouseEnter(testParamCell, { bubbles: true }) jest.advanceTimersByTime(CELL_TOOLTIP_DELAY - 1) @@ -637,37 +634,34 @@ describe('App', () => { const tooltip = screen.getByRole('tooltip') expect(tooltip).toBeInTheDocument() - expect(tooltip).toHaveTextContent(testParamStringValue) + expect(tooltip).toHaveTextContent(cellLabel) fireEvent.mouseLeave(testParamCell, { bubbles: true }) - jest.advanceTimersByTime(1) + jest.advanceTimersByTime(CELL_TOOLTIP_DELAY) expect(screen.queryByRole('tooltip')).not.toBeInTheDocument() - - jest.mocked(window.requestAnimationFrame).mockRestore() }) - it('should show the expected tooltip for all values in the data types fixture', async () => { - jest - .spyOn(window, 'requestAnimationFrame') - .mockImplementation(cb => window.setTimeout(cb, 1)) - const expectTooltipValue: (args: { - cellLabel: string - expectedTooltipResult: string - }) => Promise = async ({ cellLabel, expectedTooltipResult }) => { - const testCell = screen.getAllByText(cellLabel)?.[0] - fireEvent.mouseEnter(testCell, { bubbles: true }) - jest.advanceTimersByTime(CELL_TOOLTIP_DELAY) - const tooltip = await screen.findByRole('tooltip') - expect(tooltip).toHaveTextContent(expectedTooltipResult) - jest.useRealTimers() - await new Promise(resolve => setTimeout(resolve, 1)) - fireEvent.mouseLeave(testCell, { bubbles: true }) - await new Promise(resolve => setTimeout(resolve, 1)) - jest.useFakeTimers() - expect(screen.queryByRole('tooltip')).not.toBeInTheDocument() - } + 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() + } + it('should show the expected tooltip for all data types', () => { render() fireEvent( window, @@ -679,23 +673,22 @@ describe('App', () => { }) ) - await expectTooltipValue({ + expectTooltipValue({ cellLabel: '1.9293', expectedTooltipResult: '1.9293040037155151' }) - await expectTooltipValue({ + expectTooltipValue({ cellLabel: 'true', expectedTooltipResult: 'true' }) - await expectTooltipValue({ + expectTooltipValue({ cellLabel: 'false', expectedTooltipResult: 'false' }) - await expectTooltipValue({ + expectTooltipValue({ cellLabel: '[true, false, string, 2]', expectedTooltipResult: '[true, false, string, 2]' }) - jest.mocked(window.requestAnimationFrame).mockRestore() }) }) From 124217b3e6e8cd698c733859b76d3b3eddcee226 Mon Sep 17 00:00:00 2001 From: rogermparent Date: Fri, 8 Jul 2022 00:21:59 -0400 Subject: [PATCH 08/22] Fix grammar of rows tests --- extension/src/experiments/model/index.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extension/src/experiments/model/index.test.ts b/extension/src/experiments/model/index.test.ts index 23c9d5a021..b7e9a191fc 100644 --- a/extension/src/experiments/model/index.test.ts +++ b/extension/src/experiments/model/index.test.ts @@ -74,13 +74,13 @@ describe('ExperimentsModel', () => { expect(model.getRowData()).toStrictEqual(rowsFixture) }) - it('should return expected rows the deeply nested output fixture', () => { + it('should return expected rows when given the deeply nested output fixture', () => { const model = new ExperimentsModel('', buildMockMemento()) model.transformAndSet(deeplyNestedOutput) expect(model.getRowData()).toStrictEqual(deeplyNestedRows) }) - it('should return expected rows the data types output fixture', () => { + it('should return expected rows when given the data types output fixture', () => { const model = new ExperimentsModel('', buildMockMemento()) model.transformAndSet(dataTypesOutput) expect(model.getRowData()).toStrictEqual(dataTypesRows) From efee628a4df4cf507569e3d6e2bbe2e75e9b08e5 Mon Sep 17 00:00:00 2001 From: rogermparent Date: Fri, 8 Jul 2022 00:24:07 -0400 Subject: [PATCH 09/22] Make text of columns tests similar to rows tests --- extension/src/experiments/columns/model.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/extension/src/experiments/columns/model.test.ts b/extension/src/experiments/columns/model.test.ts index 3caa77d951..8a06b5aa6b 100644 --- a/extension/src/experiments/columns/model.test.ts +++ b/extension/src/experiments/columns/model.test.ts @@ -18,19 +18,19 @@ import { describe('ColumnsModel', () => { const exampleDvcRoot = 'test' - it('should return columns that equal the default columns fixture when given the default output fixture', async () => { + it('should return expected columns when given the default output fixture', async () => { const model = new ColumnsModel('', buildMockMemento()) await model.transformAndSet(outputFixture) expect(model.getSelected()).toStrictEqual(columnsFixture) }) - it('should return expected columns that equal the deeply nested output fixture', async () => { + it('should return expected columns when given the deeply nested output fixture', async () => { const model = new ColumnsModel('', buildMockMemento()) await model.transformAndSet(deeplyNestedOutput) expect(model.getSelected()).toStrictEqual(deeplyNestedColumns) }) - it('should return expected columns that equal the data types output fixture', async () => { + it('should return expected columns when given the data types output fixture', async () => { const model = new ColumnsModel('', buildMockMemento()) await model.transformAndSet(dataTypesOutput) expect(model.getSelected()).toStrictEqual(dataTypesColumns) From 570038ff490d5938a9563094d625ed2f1932e8fe Mon Sep 17 00:00:00 2001 From: rogermparent Date: Fri, 8 Jul 2022 00:34:46 -0400 Subject: [PATCH 10/22] Move expectTooltipValue in only function it's used, remove module-wide exception --- .../src/experiments/components/App.test.tsx | 32 +++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/webview/src/experiments/components/App.test.tsx b/webview/src/experiments/components/App.test.tsx index 62b87eaf5e..c553c630db 100644 --- a/webview/src/experiments/components/App.test.tsx +++ b/webview/src/experiments/components/App.test.tsx @@ -1,7 +1,7 @@ /** * @jest-environment jsdom */ -/* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "expectHeaders", "expectTooltipValue"] }] */ +/* eslint jest/expect-expect: ["error", { "assertFunctionNames": ["expect", "expectHeaders"] }] */ import React from 'react' import { cleanup, @@ -642,26 +642,26 @@ describe('App', () => { expect(screen.queryByRole('tooltip')).not.toBeInTheDocument() }) - const expectTooltipValue: (args: { - cellLabel: string - expectedTooltipResult: string - }) => void = ({ cellLabel, expectedTooltipResult }) => { - const testParamCell = screen.getByText(cellLabel) - expect(testParamCell).toBeInTheDocument() + 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 }) + fireEvent.mouseEnter(testParamCell, { bubbles: true }) - jest.advanceTimersByTime(CELL_TOOLTIP_DELAY) - const tooltip = screen.queryByRole('tooltip') - expect(tooltip).toHaveTextContent(expectedTooltipResult) + jest.advanceTimersByTime(CELL_TOOLTIP_DELAY) + const tooltip = screen.queryByRole('tooltip') + expect(tooltip).toHaveTextContent(expectedTooltipResult) - fireEvent.mouseLeave(testParamCell, { bubbles: true }) + fireEvent.mouseLeave(testParamCell, { bubbles: true }) - jest.advanceTimersByTime(CELL_TOOLTIP_DELAY) - expect(screen.queryByRole('tooltip')).not.toBeInTheDocument() - } + jest.advanceTimersByTime(CELL_TOOLTIP_DELAY) + expect(screen.queryByRole('tooltip')).not.toBeInTheDocument() + } - it('should show the expected tooltip for all data types', () => { render() fireEvent( window, From a7c944c1e6deada9fbcb11848bd3042234ce279b Mon Sep 17 00:00:00 2001 From: rogermparent Date: Fri, 8 Jul 2022 00:37:40 -0400 Subject: [PATCH 11/22] fix typo --- webview/src/experiments/components/App.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webview/src/experiments/components/App.test.tsx b/webview/src/experiments/components/App.test.tsx index c553c630db..ddd15cce87 100644 --- a/webview/src/experiments/components/App.test.tsx +++ b/webview/src/experiments/components/App.test.tsx @@ -155,7 +155,7 @@ describe('App', () => { ) } - it('should apply the sortingHeaderCellAsc class to) only a top level placeholder', () => { + it('should apply the sortingHeaderCellAsc class to only a top level placeholder', () => { renderTableWithPlaceholder() const topPlaceholder = screen.getByTestId( From 489eac437abe6cdbc26fe395208d5841c446ff16 Mon Sep 17 00:00:00 2001 From: rogermparent Date: Fri, 8 Jul 2022 00:52:19 -0400 Subject: [PATCH 12/22] Remove unnecessary abstraction --- .../src/experiments/components/App.test.tsx | 177 +++++++++--------- 1 file changed, 89 insertions(+), 88 deletions(-) diff --git a/webview/src/experiments/components/App.test.tsx b/webview/src/experiments/components/App.test.tsx index ddd15cce87..9dec08f43a 100644 --- a/webview/src/experiments/components/App.test.tsx +++ b/webview/src/experiments/components/App.test.tsx @@ -61,85 +61,6 @@ afterEach(() => { cleanup() }) -const sourceFilenames: Record = { - [ColumnType.PARAMS]: 'params.yaml', - [ColumnType.METRICS]: 'metrics.json', - [ColumnType.DEPS]: 'data' -} - -const buildTestColumn = ({ - name, - type: columnType = ColumnType.PARAMS, - sourceFilename = sourceFilenames[columnType], - ...rest -}: Partial & { - name: string - columnType?: ColumnType - sourceFilename?: string -}): Column => ({ - hasChildren: false, - label: name, - parentPath: buildMetricOrParamPath(columnType, sourceFilename), - path: buildMetricOrParamPath(columnType, sourceFilename, name), - pathArray: [columnType, sourceFilename, name], - type: columnType, - ...rest -}) - -const buildTestData: ( - args: { - value: T - name?: string - otherValue?: T - types?: string[] - } & Partial -) => TableData = ({ - value, - name = 'test-param', - types = [typeof value], - ...rest -}) => { - return { - ...tableDataFixture, - columns: [ - { - hasChildren: true, - label: 'summary.json', - parentPath: buildMetricOrParamPath(ColumnType.METRICS), - path: buildMetricOrParamPath(ColumnType.METRICS, 'summary.json'), - type: ColumnType.METRICS - }, - { - hasChildren: true, - label: 'params.yaml', - parentPath: ColumnType.PARAMS, - path: buildMetricOrParamPath(ColumnType.PARAMS, 'params.yaml'), - type: ColumnType.PARAMS - }, - buildTestColumn({ - name, - types, - ...rest - }) - ], - rows: [ - { - id: 'workspace', - label: 'workspace', - params: { - 'params.yaml': { - [name]: value - } - } - }, - { - id: 'main', - label: 'main' - } - ] - } -} - describe('App', () => { describe('Sorting Classes', () => { const renderTableWithPlaceholder = () => { @@ -543,13 +464,93 @@ describe('App', () => { jest.useRealTimers() }) - const testParamName = 'test-param' - const cellLabel = 'Test String' - const testData = buildTestData({ - maxStringLength: 10, - name: testParamName, - value: cellLabel - }) + const testParamName = 'test_param_with_long_name' + const testParamPath = buildMetricOrParamPath( + ColumnType.PARAMS, + 'params.yaml', + testParamName + ) + const testParamStringValue = 'Test Value' + const testMetricNumberValue = 1.9293040037155151 + + const testData = { + ...tableDataFixture, + columns: [ + { + hasChildren: true, + label: 'summary.json', + parentPath: buildMetricOrParamPath(ColumnType.METRICS), + path: buildMetricOrParamPath(ColumnType.METRICS, 'summary.json'), + type: ColumnType.METRICS + }, + { + hasChildren: false, + label: 'loss', + maxNumber: testMetricNumberValue, + maxStringLength: 18, + minNumber: testMetricNumberValue, + parentPath: buildMetricOrParamPath( + ColumnType.METRICS, + 'summary.json' + ), + path: buildMetricOrParamPath( + ColumnType.METRICS, + 'summary.json', + 'loss' + ), + pathArray: [ColumnType.METRICS, 'summary.json', 'loss'], + type: ColumnType.METRICS, + types: ['number'] + }, + { + hasChildren: true, + label: 'params.yaml', + parentPath: ColumnType.PARAMS, + path: buildMetricOrParamPath(ColumnType.PARAMS, 'params.yaml'), + type: ColumnType.PARAMS + }, + { + hasChildren: false, + label: testParamName, + maxStringLength: 10, + parentPath: buildMetricOrParamPath(ColumnType.PARAMS, 'params.yaml'), + path: testParamPath, + pathArray: [ColumnType.PARAMS, 'params.yaml', testParamName], + type: ColumnType.PARAMS, + types: ['string'] + } + ], + rows: [ + { + id: 'workspace', + label: 'workspace', + metrics: { + 'summary.json': { + loss: testMetricNumberValue + } + }, + params: { + 'params.yaml': { + [testParamName]: testParamStringValue + } + } + }, + { + id: 'main', + label: 'main', + metrics: { + 'summary.json': { + loss: testMetricNumberValue + 1 + } + }, + params: { + 'params.yaml': { + [testParamName]: 'Other Value' + } + } + } + ] + } it('should show and hide a tooltip on mouseEnter and mouseLeave of a header', () => { mockedUseIsFullyContained.mockReturnValue(false) @@ -624,7 +625,7 @@ describe('App', () => { }) ) - const testParamCell = screen.getByText(cellLabel) + const testParamCell = screen.getByText(testParamStringValue) fireEvent.mouseEnter(testParamCell, { bubbles: true }) jest.advanceTimersByTime(CELL_TOOLTIP_DELAY - 1) @@ -634,7 +635,7 @@ describe('App', () => { const tooltip = screen.getByRole('tooltip') expect(tooltip).toBeInTheDocument() - expect(tooltip).toHaveTextContent(cellLabel) + expect(tooltip).toHaveTextContent(testParamStringValue) fireEvent.mouseLeave(testParamCell, { bubbles: true }) From 74bc55c2e87d95c86c73946a2aef5ed2399cea2c Mon Sep 17 00:00:00 2001 From: rogermparent Date: Fri, 8 Jul 2022 01:18:30 -0400 Subject: [PATCH 13/22] Add interactive cell tooltip test --- .../src/experiments/components/App.test.tsx | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/webview/src/experiments/components/App.test.tsx b/webview/src/experiments/components/App.test.tsx index 9dec08f43a..528ea9d4ef 100644 --- a/webview/src/experiments/components/App.test.tsx +++ b/webview/src/experiments/components/App.test.tsx @@ -643,6 +643,32 @@ describe('App', () => { expect(screen.queryByRole('tooltip')).not.toBeInTheDocument() }) + it('should persist a cell tooltip when it is moused into', () => { + render() + fireEvent( + window, + new MessageEvent('message', { + data: { + data: testData, + type: MessageToWebviewType.SET_DATA + } + }) + ) + + const testParamCell = screen.getByText(testParamStringValue) + fireEvent.mouseEnter(testParamCell, { bubbles: true }) + + jest.advanceTimersByTime(CELL_TOOLTIP_DELAY) + const tooltip = screen.getByRole('tooltip') + expect(tooltip).toBeInTheDocument() + + fireEvent.mouseLeave(testParamCell, { bubbles: true }) + fireEvent.mouseEnter(tooltip, { bubbles: true }) + + jest.advanceTimersByTime(CELL_TOOLTIP_DELAY) + expect(tooltip).toBeInTheDocument() + }) + it('should show the expected tooltip for all data types', () => { const expectTooltipValue: (args: { cellLabel: string From d942f621e4fc3fb3829f439db6169ec64c0dcb6a Mon Sep 17 00:00:00 2001 From: rogermparent Date: Fri, 8 Jul 2022 10:18:46 -0400 Subject: [PATCH 14/22] Change test wording --- extension/src/experiments/columns/model.test.ts | 6 +++--- extension/src/experiments/model/index.test.ts | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/extension/src/experiments/columns/model.test.ts b/extension/src/experiments/columns/model.test.ts index 8a06b5aa6b..22c6b888be 100644 --- a/extension/src/experiments/columns/model.test.ts +++ b/extension/src/experiments/columns/model.test.ts @@ -18,19 +18,19 @@ import { describe('ColumnsModel', () => { const exampleDvcRoot = 'test' - it('should return expected columns when given the default output fixture', async () => { + it('should return the expected columns when given the default output fixture', async () => { const model = new ColumnsModel('', buildMockMemento()) await model.transformAndSet(outputFixture) expect(model.getSelected()).toStrictEqual(columnsFixture) }) - it('should return expected columns when given the deeply nested output fixture', async () => { + it('should return the expected columns when given the deeply nested output fixture', async () => { const model = new ColumnsModel('', buildMockMemento()) await model.transformAndSet(deeplyNestedOutput) expect(model.getSelected()).toStrictEqual(deeplyNestedColumns) }) - it('should return expected columns when given the data types output fixture', async () => { + it('should return the expected columns when given the data types output fixture', async () => { const model = new ColumnsModel('', buildMockMemento()) await model.transformAndSet(dataTypesOutput) expect(model.getSelected()).toStrictEqual(dataTypesColumns) diff --git a/extension/src/experiments/model/index.test.ts b/extension/src/experiments/model/index.test.ts index b7e9a191fc..c593850c36 100644 --- a/extension/src/experiments/model/index.test.ts +++ b/extension/src/experiments/model/index.test.ts @@ -68,19 +68,19 @@ describe('ExperimentsModel', () => { return { data } } - it('should return expected rows when given the output fixture', () => { + it('should return the expected rows when given the output fixture', () => { const model = new ExperimentsModel('', buildMockMemento()) model.transformAndSet(outputFixture) expect(model.getRowData()).toStrictEqual(rowsFixture) }) - it('should return expected rows when given the deeply nested output fixture', () => { + it('should return the expected rows when given the deeply nested output fixture', () => { const model = new ExperimentsModel('', buildMockMemento()) model.transformAndSet(deeplyNestedOutput) expect(model.getRowData()).toStrictEqual(deeplyNestedRows) }) - it('should return expected rows when given the data types output fixture', () => { + it('should return the expected rows when given the data types output fixture', () => { const model = new ExperimentsModel('', buildMockMemento()) model.transformAndSet(dataTypesOutput) expect(model.getRowData()).toStrictEqual(dataTypesRows) From b575781e222cd5c1e4327dbaa14456bc20821284 Mon Sep 17 00:00:00 2001 From: rogermparent Date: Fri, 8 Jul 2022 10:43:35 -0400 Subject: [PATCH 15/22] Use @storybook/testing-library for Storybook play functions This should make writing play functions easier, since we can use the full testing-library query suite --- webview/package.json | 1 + .../src/stories/ComparisonTable.stories.tsx | 9 ++++--- webview/src/stories/Plots.stories.tsx | 20 +++++++--------- yarn.lock | 24 ++++++++++++++++--- 4 files changed, 35 insertions(+), 19 deletions(-) diff --git a/webview/package.json b/webview/package.json index 26362fd627..f000def4c2 100644 --- a/webview/package.json +++ b/webview/package.json @@ -23,6 +23,7 @@ }, "dependencies": { "@reduxjs/toolkit": "^1.8.2", + "@storybook/testing-library": "^0.0.13", "@tippyjs/react": "^4.2.6", "@vscode/webview-ui-toolkit": "^1.0.0", "classnames": "^2.2.6", diff --git a/webview/src/stories/ComparisonTable.stories.tsx b/webview/src/stories/ComparisonTable.stories.tsx index cd2616572d..687e661873 100644 --- a/webview/src/stories/ComparisonTable.stories.tsx +++ b/webview/src/stories/ComparisonTable.stories.tsx @@ -1,6 +1,6 @@ import { Meta, Story } from '@storybook/react/types-6-0' import { configureStore } from '@reduxjs/toolkit' -import { within } from '@testing-library/react' +import { screen, userEvent, within } from '@storybook/testing-library' import React from 'react' import { Provider, useDispatch } from 'react-redux' import plotsRevisionsFixture from 'dvc/src/test/fixtures/plotsDiff/revisions' @@ -62,12 +62,11 @@ export const WithPinnedColumn = Template.bind({}) WithPinnedColumn.parameters = { chromatic: { delay: 300 } } -WithPinnedColumn.play = async ({ canvasElement }) => { - const canvas = within(canvasElement) - const mainHeader = await canvas.findByTestId('main-header') +WithPinnedColumn.play = async () => { + const mainHeader = await screen.findByTestId('main-header') const pin = within(mainHeader).getByRole('button') - pin.click() + userEvent.click(pin) } const removeSingleImage = ( diff --git a/webview/src/stories/Plots.stories.tsx b/webview/src/stories/Plots.stories.tsx index cc74f66ef0..ad42643d3f 100644 --- a/webview/src/stories/Plots.stories.tsx +++ b/webview/src/stories/Plots.stories.tsx @@ -2,7 +2,7 @@ import { configureStore } from '@reduxjs/toolkit' import React from 'react' import { Provider, useDispatch } from 'react-redux' import { Story, Meta } from '@storybook/react/types-6-0' -import { within, waitFor } from '@testing-library/react' +import { screen, userEvent, within } from '@storybook/testing-library' import { PlotsData, DEFAULT_SECTION_COLLAPSED, @@ -196,22 +196,20 @@ export const ZoomedInPlot = Template.bind({}) ZoomedInPlot.parameters = { chromatic: { delay: 300 } } -ZoomedInPlot.play = async ({ canvasElement }) => { - const canvas = within(canvasElement) - const plots = await canvas.findAllByTestId(/^plot_/) - const plot = await waitFor(() => within(plots[0]).getByRole('button')) +ZoomedInPlot.play = async () => { + const plots = await screen.findAllByTestId(/^plot_/) + const plot = await within(plots[0]).findByRole('button') - plot.click() + userEvent.click(plot) } export const MultiviewZoomedInPlot = Template.bind({}) MultiviewZoomedInPlot.parameters = { chromatic: { delay: 300 } } -MultiviewZoomedInPlot.play = async ({ canvasElement }) => { - const canvas = within(canvasElement) - const plot = await canvas.findByTestId('plots-section_template-multi_1') - const plotButton = await waitFor(() => within(plot).getByRole('button')) +MultiviewZoomedInPlot.play = async () => { + const plot = await screen.findByTestId('plots-section_template-multi_1') + const plotButton = await within(plot).findByRole('button') - plotButton.click() + userEvent.click(plotButton) } diff --git a/yarn.lock b/yarn.lock index 0e7fd7d5b2..e421f7972a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2470,7 +2470,7 @@ ts-dedent "^2.0.0" util-deprecate "^1.0.2" -"@storybook/client-logger@6.5.9": +"@storybook/client-logger@6.5.9", "@storybook/client-logger@^6.4.0": version "6.5.9" resolved "https://registry.yarnpkg.com/@storybook/client-logger/-/client-logger-6.5.9.tgz#dc1669abe8c45af1cc38f74c6f4b15ff33e63014" integrity sha512-DOHL6p0uiDd3gV/Sb2FR+Vh6OiPrrf8BrA06uvXWsMRIIvEEvnparxv9EvPg7FlmUX0T3nq7d3juwjx4F8Wbcg== @@ -2682,7 +2682,7 @@ lodash "^4.17.21" regenerator-runtime "^0.13.7" -"@storybook/instrumenter@6.5.9": +"@storybook/instrumenter@6.5.9", "@storybook/instrumenter@^6.4.0": version "6.5.9" resolved "https://registry.yarnpkg.com/@storybook/instrumenter/-/instrumenter-6.5.9.tgz#885d9dec31b7b7fa6ea29b446105480450e527b8" integrity sha512-I2nu/6H0MAy8d+d3LY/G6oYEFyWlc8f2Qs2DhpYh5FiCgIpzvY0DMN05Lf8oaXdKHL3lPF/YLJH17FttekXs1w== @@ -2962,6 +2962,17 @@ read-pkg-up "^7.0.1" regenerator-runtime "^0.13.7" +"@storybook/testing-library@^0.0.13": + version "0.0.13" + resolved "https://registry.yarnpkg.com/@storybook/testing-library/-/testing-library-0.0.13.tgz#417c87d4ea62895092ec5fdf67027ae201254f45" + integrity sha512-vRMeIGer4EjJkTgI8sQyK9W431ekPWYCWL//OmSDJ64IT3h7FnW7Xg6p+eqM3oII98/O5pcya5049GxnjaPtxw== + dependencies: + "@storybook/client-logger" "^6.4.0" + "@storybook/instrumenter" "^6.4.0" + "@testing-library/dom" "^8.3.0" + "@testing-library/user-event" "^13.2.1" + ts-dedent "^2.2.0" + "@storybook/theming@6.5.9": version "6.5.9" resolved "https://registry.yarnpkg.com/@storybook/theming/-/theming-6.5.9.tgz#13f60a3a3cd73ceb5caf9f188e1627e79f1891aa" @@ -3112,7 +3123,7 @@ dependencies: defer-to-connect "^2.0.0" -"@testing-library/dom@^8.5.0": +"@testing-library/dom@^8.3.0", "@testing-library/dom@^8.5.0": version "8.14.0" resolved "https://registry.yarnpkg.com/@testing-library/dom/-/dom-8.14.0.tgz#c9830a21006d87b9ef6e1aae306cf49b0283e28e" integrity sha512-m8FOdUo77iMTwVRCyzWcqxlEIk+GnopbrRI15a0EaLbpZSCinIVI4kSQzWhkShK83GogvEFJSsHF3Ws0z1vrqA== @@ -3150,6 +3161,13 @@ "@testing-library/dom" "^8.5.0" "@types/react-dom" "^18.0.0" +"@testing-library/user-event@^13.2.1": + version "13.5.0" + resolved "https://registry.yarnpkg.com/@testing-library/user-event/-/user-event-13.5.0.tgz#69d77007f1e124d55314a2b73fd204b333b13295" + integrity sha512-5Kwtbo3Y/NowpkbRuSepbyMFkZmHgD+vPzYB/RJ4oxt5Gj/avFFBYjhw27cqSVPVw/3a67NK1PbiIr9k4Gwmdg== + dependencies: + "@babel/runtime" "^7.12.5" + "@tippyjs/react@^4.2.6": version "4.2.6" resolved "https://registry.yarnpkg.com/@tippyjs/react/-/react-4.2.6.tgz#971677a599bf663f20bb1c60a62b9555b749cc71" From 22b25464ec0aa8f9690721ae432796ce9c0b7f25 Mon Sep 17 00:00:00 2001 From: rogermparent Date: Fri, 8 Jul 2022 10:53:05 -0400 Subject: [PATCH 16/22] Stop using screen --- webview/src/stories/ComparisonTable.stories.tsx | 6 +++--- webview/src/stories/Plots.stories.tsx | 12 +++++++----- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/webview/src/stories/ComparisonTable.stories.tsx b/webview/src/stories/ComparisonTable.stories.tsx index 687e661873..7379331735 100644 --- a/webview/src/stories/ComparisonTable.stories.tsx +++ b/webview/src/stories/ComparisonTable.stories.tsx @@ -1,6 +1,6 @@ import { Meta, Story } from '@storybook/react/types-6-0' import { configureStore } from '@reduxjs/toolkit' -import { screen, userEvent, within } from '@storybook/testing-library' +import { userEvent, within } from '@storybook/testing-library' import React from 'react' import { Provider, useDispatch } from 'react-redux' import plotsRevisionsFixture from 'dvc/src/test/fixtures/plotsDiff/revisions' @@ -62,8 +62,8 @@ export const WithPinnedColumn = Template.bind({}) WithPinnedColumn.parameters = { chromatic: { delay: 300 } } -WithPinnedColumn.play = async () => { - const mainHeader = await screen.findByTestId('main-header') +WithPinnedColumn.play = async ({ canvasElement }) => { + const mainHeader = await within(canvasElement).findByTestId('main-header') const pin = within(mainHeader).getByRole('button') userEvent.click(pin) diff --git a/webview/src/stories/Plots.stories.tsx b/webview/src/stories/Plots.stories.tsx index ad42643d3f..7b5e56faa1 100644 --- a/webview/src/stories/Plots.stories.tsx +++ b/webview/src/stories/Plots.stories.tsx @@ -2,7 +2,7 @@ import { configureStore } from '@reduxjs/toolkit' import React from 'react' import { Provider, useDispatch } from 'react-redux' import { Story, Meta } from '@storybook/react/types-6-0' -import { screen, userEvent, within } from '@storybook/testing-library' +import { userEvent, within } from '@storybook/testing-library' import { PlotsData, DEFAULT_SECTION_COLLAPSED, @@ -196,8 +196,8 @@ export const ZoomedInPlot = Template.bind({}) ZoomedInPlot.parameters = { chromatic: { delay: 300 } } -ZoomedInPlot.play = async () => { - const plots = await screen.findAllByTestId(/^plot_/) +ZoomedInPlot.play = async ({ canvasElement }) => { + const plots = await within(canvasElement).findAllByTestId(/^plot_/) const plot = await within(plots[0]).findByRole('button') userEvent.click(plot) @@ -207,8 +207,10 @@ export const MultiviewZoomedInPlot = Template.bind({}) MultiviewZoomedInPlot.parameters = { chromatic: { delay: 300 } } -MultiviewZoomedInPlot.play = async () => { - const plot = await screen.findByTestId('plots-section_template-multi_1') +MultiviewZoomedInPlot.play = async ({ canvasElement }) => { + const plot = await within(canvasElement).findByTestId( + 'plots-section_template-multi_1' + ) const plotButton = await within(plot).findByRole('button') userEvent.click(plotButton) From c74e3b13b0c72dd465962beb24cb52e651771020 Mon Sep 17 00:00:00 2001 From: rogermparent Date: Fri, 8 Jul 2022 11:09:35 -0400 Subject: [PATCH 17/22] Add .play to show bool tooltip on data types Storybook --- webview/src/stories/Table.stories.tsx | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/webview/src/stories/Table.stories.tsx b/webview/src/stories/Table.stories.tsx index c53b595031..d28710773d 100644 --- a/webview/src/stories/Table.stories.tsx +++ b/webview/src/stories/Table.stories.tsx @@ -6,10 +6,12 @@ import { TableData } from 'dvc/src/experiments/webview/contract' import workspaceChangesFixture from 'dvc/src/test/fixtures/expShow/workspaceChanges' import deeplyNestedTableData from 'dvc/src/test/fixtures/expShow/deeplyNested' import { dataTypesTableData } from 'dvc/src/test/fixtures/expShow/dataTypes' +import { within, userEvent } from '@storybook/testing-library' import Experiments from '../experiments/components/Experiments' import './test-vscode-styles.scss' import '../shared/style.scss' +import { CELL_TOOLTIP_DELAY } from '../shared/components/tooltip/Tooltip' const tableData: TableData = { changes: workspaceChangesFixture, @@ -82,6 +84,13 @@ WithNoRunningExperiments.args = { export const WithAllDataTypes = Template.bind({}) WithAllDataTypes.args = { tableData: dataTypesTableData } +WithAllDataTypes.play = async ({ canvasElement }) => { + const falseCell = await within(canvasElement).findByText('false') + userEvent.hover(falseCell, { bubbles: true }) +} +WithAllDataTypes.parameters = { + chromatic: { delay: CELL_TOOLTIP_DELAY } +} export const WithDeeplyNestedHeaders = Template.bind({}) WithDeeplyNestedHeaders.args = { tableData: deeplyNestedTableData } From c638db3e2794ff8eb6686fc1f1451f04a98994f7 Mon Sep 17 00:00:00 2001 From: rogermparent Date: Fri, 8 Jul 2022 11:20:25 -0400 Subject: [PATCH 18/22] Increase chromatic delay for multiview zoomed plot --- webview/src/stories/Plots.stories.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webview/src/stories/Plots.stories.tsx b/webview/src/stories/Plots.stories.tsx index 7b5e56faa1..80d05d73e2 100644 --- a/webview/src/stories/Plots.stories.tsx +++ b/webview/src/stories/Plots.stories.tsx @@ -205,7 +205,7 @@ ZoomedInPlot.play = async ({ canvasElement }) => { export const MultiviewZoomedInPlot = Template.bind({}) MultiviewZoomedInPlot.parameters = { - chromatic: { delay: 300 } + chromatic: { delay: 400 } } MultiviewZoomedInPlot.play = async ({ canvasElement }) => { const plot = await within(canvasElement).findByTestId( From 81124789a05bad3cb30f670a94fba2c52a6712f6 Mon Sep 17 00:00:00 2001 From: rogermparent Date: Fri, 8 Jul 2022 11:28:21 -0400 Subject: [PATCH 19/22] Further increase chromatic delay --- webview/src/stories/Plots.stories.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/webview/src/stories/Plots.stories.tsx b/webview/src/stories/Plots.stories.tsx index 80d05d73e2..5186c90c12 100644 --- a/webview/src/stories/Plots.stories.tsx +++ b/webview/src/stories/Plots.stories.tsx @@ -205,7 +205,7 @@ ZoomedInPlot.play = async ({ canvasElement }) => { export const MultiviewZoomedInPlot = Template.bind({}) MultiviewZoomedInPlot.parameters = { - chromatic: { delay: 400 } + chromatic: { delay: 1000 } } MultiviewZoomedInPlot.play = async ({ canvasElement }) => { const plot = await within(canvasElement).findByTestId( From 2292037738f8a2dcf4e83d9592660a13424954d9 Mon Sep 17 00:00:00 2001 From: rogermparent Date: Mon, 11 Jul 2022 20:38:23 -0400 Subject: [PATCH 20/22] Remove chromatic delay on MultiView Zoomed plot --- webview/src/stories/Plots.stories.tsx | 3 --- 1 file changed, 3 deletions(-) diff --git a/webview/src/stories/Plots.stories.tsx b/webview/src/stories/Plots.stories.tsx index 7b5e56faa1..833333cb49 100644 --- a/webview/src/stories/Plots.stories.tsx +++ b/webview/src/stories/Plots.stories.tsx @@ -204,9 +204,6 @@ ZoomedInPlot.play = async ({ canvasElement }) => { } export const MultiviewZoomedInPlot = Template.bind({}) -MultiviewZoomedInPlot.parameters = { - chromatic: { delay: 300 } -} MultiviewZoomedInPlot.play = async ({ canvasElement }) => { const plot = await within(canvasElement).findByTestId( 'plots-section_template-multi_1' From b20d192033cc4a2e1f0a81369af4306f31d0a2db Mon Sep 17 00:00:00 2001 From: rogermparent Date: Mon, 11 Jul 2022 21:22:46 -0400 Subject: [PATCH 21/22] Remove chromatic delays --- webview/src/stories/ComparisonTable.stories.tsx | 3 --- webview/src/stories/Plots.stories.tsx | 3 --- 2 files changed, 6 deletions(-) diff --git a/webview/src/stories/ComparisonTable.stories.tsx b/webview/src/stories/ComparisonTable.stories.tsx index 7379331735..69035dadec 100644 --- a/webview/src/stories/ComparisonTable.stories.tsx +++ b/webview/src/stories/ComparisonTable.stories.tsx @@ -59,9 +59,6 @@ const Template: Story = ({ plots, revisions }) => { export const Basic = Template.bind({}) export const WithPinnedColumn = Template.bind({}) -WithPinnedColumn.parameters = { - chromatic: { delay: 300 } -} WithPinnedColumn.play = async ({ canvasElement }) => { const mainHeader = await within(canvasElement).findByTestId('main-header') const pin = within(mainHeader).getByRole('button') diff --git a/webview/src/stories/Plots.stories.tsx b/webview/src/stories/Plots.stories.tsx index 833333cb49..4825f3ac6f 100644 --- a/webview/src/stories/Plots.stories.tsx +++ b/webview/src/stories/Plots.stories.tsx @@ -193,9 +193,6 @@ VirtualizedPlots.args = { VirtualizedPlots.parameters = chromaticParameters export const ZoomedInPlot = Template.bind({}) -ZoomedInPlot.parameters = { - chromatic: { delay: 300 } -} ZoomedInPlot.play = async ({ canvasElement }) => { const plots = await within(canvasElement).findAllByTestId(/^plot_/) const plot = await within(plots[0]).findByRole('button') From db47df5a7b1d0da979bac52c063a6762c3733f72 Mon Sep 17 00:00:00 2001 From: rogermparent Date: Mon, 11 Jul 2022 21:28:32 -0400 Subject: [PATCH 22/22] Await multiview plot --- webview/src/stories/Plots.stories.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/webview/src/stories/Plots.stories.tsx b/webview/src/stories/Plots.stories.tsx index 4825f3ac6f..be1df04a5d 100644 --- a/webview/src/stories/Plots.stories.tsx +++ b/webview/src/stories/Plots.stories.tsx @@ -205,6 +205,7 @@ MultiviewZoomedInPlot.play = async ({ canvasElement }) => { const plot = await within(canvasElement).findByTestId( 'plots-section_template-multi_1' ) + await within(plot).findByRole('graphics-document') const plotButton = await within(plot).findByRole('button') userEvent.click(plotButton)