diff --git a/.eslintrc.js b/.eslintrc.js index cfcb12dafd..18ecf002ff 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -27,7 +27,8 @@ module.exports = { '@typescript-eslint/no-unsafe-call': 'off', '@typescript-eslint/no-unsafe-return': 'off', 'no-undef': 'off', - 'sonarjs/no-duplicate-string': 'off' + 'sonarjs/no-duplicate-string': 'off', + 'testing-library/no-render-in-setup': 'off' } }, { diff --git a/webview/package.json b/webview/package.json index ea99978a9d..467dae359c 100644 --- a/webview/package.json +++ b/webview/package.json @@ -39,7 +39,7 @@ "vega-lite": "^5.2.0" }, "devDependencies": { - "@storybook/testing-library": "^0.0.13", + "@storybook/testing-library": "0.0.13", "@storybook/addon-essentials": "6.5.9", "@storybook/addon-interactions": "6.5.9", "@storybook/addons": "6.5.9", @@ -47,7 +47,6 @@ "@storybook/manager-webpack5": "6.5.9", "@storybook/preset-scss": "1.0.3", "@storybook/react": "6.5.9", - "@storybook/testing-library": "0.0.13", "@svgr/cli": "6.3.1", "@testing-library/jest-dom": "5.16.4", "@testing-library/react": "13.3.0", diff --git a/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx index c763065f48..52c3cc24b7 100644 --- a/webview/src/plots/components/App.test.tsx +++ b/webview/src/plots/components/App.test.tsx @@ -25,7 +25,8 @@ import { PlotSize, Section, TemplatePlotGroup, - TemplatePlotsData + TemplatePlotsData, + TemplatePlotSection } from 'dvc/src/plots/webview/contract' import { MessageFromWebviewType, @@ -36,6 +37,7 @@ import { act } from 'react-dom/test-utils' import { App } from './App' import { NewSectionBlock } from './templatePlots/TemplatePlots' import { SectionDescription } from './PlotsContainer' +import { CheckpointPlotsById, plotDataStore } from './plotDataStore' import { plotsReducers } from '../store' import { vsCodeApi } from '../../shared/api' import { createBubbledEvent, dragAndDrop, dragEnter } from '../../test/dragDrop' @@ -68,40 +70,13 @@ const originalOffsetWidth = Object.getOwnPropertyDescriptor( 'offsetWidth' )?.value -beforeAll(() => { - Object.defineProperty(HTMLElement.prototype, 'offsetHeight', { - configurable: true, - value: 50 - }) - Object.defineProperty(HTMLElement.prototype, 'offsetWidth', { - configurable: true, - value: 50 - }) -}) - -beforeEach(() => { - jest.clearAllMocks() - jest - .spyOn(HTMLElement.prototype, 'clientHeight', 'get') - .mockImplementation(() => heightToSuppressVegaError) -}) - -afterEach(() => { - cleanup() -}) - -afterAll(() => { - Object.defineProperty(HTMLElement.prototype, 'offsetHeight', { - configurable: true, - value: originalOffsetHeight - }) - Object.defineProperty(HTMLElement.prototype, 'offsetHeight', { - configurable: true, - value: originalOffsetWidth - }) -}) - describe('App', () => { + const sectionPosition = { + [Section.CHECKPOINT_PLOTS]: 2, + [Section.TEMPLATE_PLOTS]: 0, + [Section.COMPARISON_TABLE]: 1 + } + const sendSetDataMessage = (data: PlotsData) => { const message = new MessageEvent('message', { data: { @@ -113,7 +88,8 @@ describe('App', () => { } const renderAppWithOptionalData = (data?: PlotsData) => { - const store = configureStore({ reducer: plotsReducers }) + const store = configureStore({ reducer: { ...plotsReducers } }) + render( @@ -140,6 +116,50 @@ describe('App', () => { ] } as TemplatePlotsData + const getCheckpointMenuItem = (position: number) => + within( + screen.getAllByTestId('plots-container')[ + sectionPosition[Section.CHECKPOINT_PLOTS] + ] + ).getAllByTestId('icon-menu-item')[position] + + const getCheckpointSizePickerButton = () => getCheckpointMenuItem(1) + + beforeAll(() => { + Object.defineProperty(HTMLElement.prototype, 'offsetHeight', { + configurable: true, + value: 50 + }) + Object.defineProperty(HTMLElement.prototype, 'offsetWidth', { + configurable: true, + value: 50 + }) + }) + + beforeEach(() => { + jest.clearAllMocks() + jest + .spyOn(HTMLElement.prototype, 'clientHeight', 'get') + .mockImplementation(() => heightToSuppressVegaError) + plotDataStore.checkpoint = {} as CheckpointPlotsById + plotDataStore.template = [] as TemplatePlotSection[] + }) + + afterEach(() => { + cleanup() + }) + + afterAll(() => { + Object.defineProperty(HTMLElement.prototype, 'offsetHeight', { + configurable: true, + value: originalOffsetHeight + }) + Object.defineProperty(HTMLElement.prototype, 'offsetHeight', { + configurable: true, + value: originalOffsetWidth + }) + }) + it('should send the initialized message on first render', () => { renderAppWithOptionalData() expect(mockPostMessage).toHaveBeenCalledWith({ @@ -192,29 +212,27 @@ describe('App', () => { }) }) - it('should render only checkpoint plots when given a message with only checkpoint plots data', () => { + it('should render other sections given a message with only checkpoint plots data', () => { renderAppWithOptionalData({ checkpoint: checkpointPlotsFixture }) expect(screen.queryByText('Loading Plots...')).not.toBeInTheDocument() expect(screen.getByText('Trends')).toBeInTheDocument() - expect(screen.queryByText('Data Series')).not.toBeInTheDocument() - expect(screen.queryByText('Images')).not.toBeInTheDocument() + expect(screen.getByText('Data Series')).toBeInTheDocument() + expect(screen.getByText('Images')).toBeInTheDocument() + expect(screen.getByText('No Plots to Display')).toBeInTheDocument() + expect(screen.getByText('No Images to Compare')).toBeInTheDocument() }) - it('should render checkpoint and template plots when given messages with both types of plots data', () => { + it('should render checkpoint even when there is no checkpoint plots data', () => { renderAppWithOptionalData({ - checkpoint: checkpointPlotsFixture - }) - - sendSetDataMessage({ template: templatePlotsFixture }) expect(screen.queryByText('Loading Plots...')).not.toBeInTheDocument() expect(screen.getByText('Trends')).toBeInTheDocument() - expect(screen.getByText('Data Series')).toBeInTheDocument() + expect(screen.getByText('No Plots to Display')).toBeInTheDocument() }) it('should render the comparison table when given a message with comparison plots data', () => { @@ -281,8 +299,7 @@ describe('App', () => { it('should toggle the visibility of plots when clicking the metrics in the metrics picker', async () => { renderAppWithOptionalData({ - checkpoint: checkpointPlotsFixture, - template: null + checkpoint: checkpointPlotsFixture }) const summaryElement = await screen.findByText('Trends') @@ -293,7 +310,7 @@ describe('App', () => { expect(screen.getByTestId('plot-summary.json:loss')).toBeInTheDocument() - const [pickerButton] = screen.queryAllByTestId('icon-menu-item') + const pickerButton = getCheckpointMenuItem(0) fireEvent.mouseEnter(pickerButton) fireEvent.click(pickerButton) @@ -326,7 +343,7 @@ describe('App', () => { checkpoint: checkpointPlotsFixture }) - const [pickerButton] = screen.getAllByTestId('icon-menu-item') + const pickerButton = getCheckpointMenuItem(0) fireEvent.mouseEnter(pickerButton) fireEvent.click(pickerButton) @@ -366,8 +383,13 @@ describe('App', () => { renderAppWithOptionalData({ checkpoint: checkpointPlotsFixture }) + const position = sectionPosition[Section.CHECKPOINT_PLOTS] + const getWrapper = async () => { + const wrappers = await screen.findAllByTestId('plots-wrapper') + return wrappers[position] + } - const sizePickerButton = screen.getAllByTestId('icon-menu-item')[1] + const sizePickerButton = getCheckpointSizePickerButton() fireEvent.mouseEnter(sizePickerButton) fireEvent.click(sizePickerButton) @@ -376,15 +398,15 @@ describe('App', () => { const largeButton = screen.getByText('Large') fireEvent.click(smallButton) - let wrapper = await screen.findByTestId('plots-wrapper') + let wrapper = await getWrapper() expect(wrapper).toHaveClass('smallPlots') fireEvent.click(regularButton) - wrapper = await screen.findByTestId('plots-wrapper') + wrapper = await getWrapper() expect(wrapper).toHaveClass('regularPlots') fireEvent.click(largeButton) - wrapper = await screen.findByTestId('plots-wrapper') + wrapper = await getWrapper() expect(wrapper).toHaveClass('largePlots') }) @@ -393,7 +415,7 @@ describe('App', () => { checkpoint: checkpointPlotsFixture }) - const sizeButton = screen.getAllByTestId('icon-menu-item')[1] + const sizeButton = getCheckpointSizePickerButton() fireEvent.mouseEnter(sizeButton) fireEvent.click(sizeButton) @@ -419,7 +441,7 @@ describe('App', () => { checkpoint: checkpointPlotsFixture }) - const sizeButton = screen.getAllByTestId('icon-menu-item')[1] + const sizeButton = getCheckpointSizePickerButton() fireEvent.mouseEnter(sizeButton) fireEvent.click(sizeButton) @@ -527,7 +549,12 @@ describe('App', () => { checkpoint: checkpointPlotsFixture }) - const [pickerButton] = screen.queryAllByTestId('icon-menu-item') + const [pickerButton] = within( + screen.getAllByTestId('plots-container')[ + sectionPosition[Section.CHECKPOINT_PLOTS] + ] + ).queryAllByTestId('icon-menu-item') + fireEvent.mouseEnter(pickerButton) fireEvent.click(pickerButton) @@ -980,32 +1007,43 @@ describe('App', () => { }) describe('Virtualization', () => { - const changeSize = async (size: string, buttonPosition: number) => { + const changeSize = async ( + size: string, + buttonPosition: number, + wrapper: HTMLElement + ) => { const sizePickerButton = - screen.getAllByTestId('icon-menu-item')[buttonPosition] + within(wrapper).getAllByTestId('icon-menu-item')[buttonPosition] fireEvent.mouseEnter(sizePickerButton) fireEvent.click(sizePickerButton) - const sizeButton = await screen.findByText(size) + const sizeButton = await within(wrapper).findByText(size) fireEvent.click(sizeButton) - await screen.findByTestId('plots-wrapper') + await screen.findAllByTestId('plots-wrapper') fireEvent.click(sizePickerButton) - await screen.findByTestId('plots-wrapper') + await screen.findAllByTestId('plots-wrapper') } const renderAppAndChangeSize = async ( data: PlotsData, size: string, - buttonPosition: number + section: Section ) => { renderAppWithOptionalData({ ...data, sectionCollapsed: DEFAULT_SECTION_COLLAPSED }) - await screen.findByTestId('plots-wrapper') - await changeSize(size, buttonPosition) + const sectionButtonPosition = { + [Section.CHECKPOINT_PLOTS]: 1, + [Section.TEMPLATE_PLOTS]: 0, + [Section.COMPARISON_TABLE]: 0 + } + const wrappers = await screen.findAllByTestId('plots-container') + const wrapper = wrappers[sectionPosition[section]] + + await changeSize(size, sectionButtonPosition[section], wrapper) } const createCheckpointPlots = (nbOfPlots: number) => { @@ -1035,7 +1073,7 @@ describe('App', () => { await renderAppAndChangeSize( { checkpoint: createCheckpointPlots(11) }, 'Large', - 1 + Section.CHECKPOINT_PLOTS ) expect(screen.getByRole('grid')).toBeInTheDocument() @@ -1044,7 +1082,7 @@ describe('App', () => { checkpoint: createCheckpointPlots(50) }) - await screen.findByTestId('plots-wrapper') + await screen.findAllByTestId('plots-wrapper') expect(screen.getByRole('grid')).toBeInTheDocument() }) @@ -1053,7 +1091,7 @@ describe('App', () => { await renderAppAndChangeSize( { checkpoint: createCheckpointPlots(10) }, 'Large', - 1 + Section.CHECKPOINT_PLOTS ) expect(screen.queryByRole('grid')).not.toBeInTheDocument() @@ -1062,7 +1100,7 @@ describe('App', () => { checkpoint: createCheckpointPlots(1) }) - await screen.findByTestId('plots-wrapper') + await screen.findAllByTestId('plots-wrapper') expect(screen.queryByRole('grid')).not.toBeInTheDocument() }) @@ -1071,7 +1109,7 @@ describe('App', () => { await renderAppAndChangeSize( { template: manyTemplatePlots(11) }, 'Large', - 0 + Section.TEMPLATE_PLOTS ) expect(screen.getByRole('grid')).toBeInTheDocument() @@ -1080,7 +1118,7 @@ describe('App', () => { template: manyTemplatePlots(50) }) - await screen.findByTestId('plots-wrapper') + await screen.findAllByTestId('plots-wrapper') expect(screen.getByRole('grid')).toBeInTheDocument() }) @@ -1089,7 +1127,7 @@ describe('App', () => { await renderAppAndChangeSize( { template: manyTemplatePlots(10) }, 'Large', - 0 + Section.TEMPLATE_PLOTS ) expect(screen.queryByRole('grid')).not.toBeInTheDocument() @@ -1098,7 +1136,7 @@ describe('App', () => { template: manyTemplatePlots(1) }) - await screen.findByTestId('plots-wrapper') + await screen.findAllByTestId('plots-wrapper') expect(screen.queryByRole('grid')).not.toBeInTheDocument() }) @@ -1107,8 +1145,11 @@ describe('App', () => { const checkpoint = createCheckpointPlots(25) beforeEach(async () => { - // eslint-disable-next-line testing-library/no-render-in-setup - await renderAppAndChangeSize({ checkpoint }, 'Large', 1) + await renderAppAndChangeSize( + { checkpoint }, + 'Large', + Section.CHECKPOINT_PLOTS + ) }) it('should render one large plot per row per 1000px of screen when the screen is larger than 2000px', () => { @@ -1160,7 +1201,7 @@ describe('App', () => { await renderAppAndChangeSize( { checkpoint: createCheckpointPlots(16) }, 'Regular', - 1 + Section.CHECKPOINT_PLOTS ) expect(screen.getByRole('grid')).toBeInTheDocument() @@ -1170,7 +1211,7 @@ describe('App', () => { await renderAppAndChangeSize( { checkpoint: createCheckpointPlots(15) }, 'Regular', - 1 + Section.CHECKPOINT_PLOTS ) expect(screen.queryByRole('grid')).not.toBeInTheDocument() @@ -1180,7 +1221,7 @@ describe('App', () => { await renderAppAndChangeSize( { template: manyTemplatePlots(16) }, 'Regular', - 0 + Section.TEMPLATE_PLOTS ) expect(screen.getByRole('grid')).toBeInTheDocument() @@ -1190,7 +1231,7 @@ describe('App', () => { await renderAppAndChangeSize( { template: manyTemplatePlots(15) }, 'Regular', - 0 + Section.TEMPLATE_PLOTS ) expect(screen.queryByRole('grid')).not.toBeInTheDocument() @@ -1200,8 +1241,11 @@ describe('App', () => { const checkpoint = createCheckpointPlots(25) beforeEach(async () => { - // eslint-disable-next-line testing-library/no-render-in-setup - await renderAppAndChangeSize({ checkpoint }, 'Regular', 1) + await renderAppAndChangeSize( + { checkpoint }, + 'Regular', + Section.CHECKPOINT_PLOTS + ) }) it('should render one regular plot per row per 800px of screen when the screen is larger than 2000px', () => { @@ -1253,7 +1297,7 @@ describe('App', () => { await renderAppAndChangeSize( { checkpoint: createCheckpointPlots(21) }, 'Small', - 1 + Section.CHECKPOINT_PLOTS ) expect(screen.getByRole('grid')).toBeInTheDocument() @@ -1263,7 +1307,7 @@ describe('App', () => { await renderAppAndChangeSize( { checkpoint: createCheckpointPlots(20) }, 'Small', - 1 + Section.CHECKPOINT_PLOTS ) expect(screen.queryByRole('grid')).not.toBeInTheDocument() @@ -1273,7 +1317,7 @@ describe('App', () => { await renderAppAndChangeSize( { template: manyTemplatePlots(21) }, 'Small', - 0 + Section.TEMPLATE_PLOTS ) expect(screen.getByRole('grid')).toBeInTheDocument() @@ -1283,7 +1327,7 @@ describe('App', () => { await renderAppAndChangeSize( { template: manyTemplatePlots(20) }, 'Small', - 0 + Section.TEMPLATE_PLOTS ) expect(screen.queryByRole('grid')).not.toBeInTheDocument() @@ -1293,8 +1337,11 @@ describe('App', () => { const checkpoint = createCheckpointPlots(25) beforeEach(async () => { - // eslint-disable-next-line testing-library/no-render-in-setup - await renderAppAndChangeSize({ checkpoint }, 'Small', 1) + await renderAppAndChangeSize( + { checkpoint }, + 'Small', + Section.CHECKPOINT_PLOTS + ) }) it('should render one small plot per row per 500px of screen when the screen is larger than 2000px', () => { @@ -1468,7 +1515,6 @@ describe('App', () => { it('should not reorder the ribbon when comparison plots are reordered', () => { renderAppWithOptionalData({ comparison: comparisonTableFixture, - selectedRevisions: plotsRevisionsFixture }) diff --git a/webview/src/plots/components/Plots.tsx b/webview/src/plots/components/Plots.tsx index e2f1f8e8e5..4476e4998d 100644 --- a/webview/src/plots/components/Plots.tsx +++ b/webview/src/plots/components/Plots.tsx @@ -55,9 +55,9 @@ const PlotsContent = () => { return ( <> - {hasTemplateData && } - {hasComparisonData && } - {hasCheckpointData && } + + + {zoomedInPlot?.plot && ( = ({ ) return ( -
+
{ diff --git a/webview/src/plots/components/checkpointPlots/CheckpointPlots.tsx b/webview/src/plots/components/checkpointPlots/CheckpointPlots.tsx index 9f6e0b385d..ce5e9bbc9d 100644 --- a/webview/src/plots/components/checkpointPlots/CheckpointPlots.tsx +++ b/webview/src/plots/components/checkpointPlots/CheckpointPlots.tsx @@ -28,7 +28,7 @@ export const CheckpointPlots: React.FC = ({ colors }) => { const [order, setOrder] = useState(plotsIds) - const { size } = useSelector((state: PlotsState) => state.checkpoint) + const { size, hasData } = useSelector((state: PlotsState) => state.checkpoint) const nbItemsPerRow = useNbItemsPerRow(size) useEffect(() => { @@ -43,6 +43,10 @@ export const CheckpointPlots: React.FC = ({ }) } + if (!hasData) { + return No Plots to Display + } + const items = order.map(plot => (
diff --git a/webview/src/plots/components/checkpointPlots/CheckpointPlotsWrapper.tsx b/webview/src/plots/components/checkpointPlots/CheckpointPlotsWrapper.tsx index 5f6e824997..d8764504bb 100644 --- a/webview/src/plots/components/checkpointPlots/CheckpointPlotsWrapper.tsx +++ b/webview/src/plots/components/checkpointPlots/CheckpointPlotsWrapper.tsx @@ -33,15 +33,20 @@ export const CheckpointPlotsWrapper: React.FC = () => { dispatch(changeSize(size)) } + const menu = + plotsIds.length > 0 + ? { + plots: metrics, + selectedPlots, + setSelectedPlots: setSelectedMetrics + } + : undefined + return ( { const { plots } = useSelector((state: PlotsState) => state.comparison) @@ -54,6 +55,10 @@ export const ComparisonTable: React.FC = () => { useEffect(() => setComparisonPlots(plots), [plots]) + if (!plots || plots.length === 0) { + return No Images to Compare + } + const setColumnsOrder = (order: string[]) => { const newOrder = reorderObjectList(order, columns, 'revision') setColumns(newOrder) diff --git a/webview/src/plots/components/plotDataStore.ts b/webview/src/plots/components/plotDataStore.ts index 4de4bac82d..9150be9662 100644 --- a/webview/src/plots/components/plotDataStore.ts +++ b/webview/src/plots/components/plotDataStore.ts @@ -3,7 +3,7 @@ import { TemplatePlotSection } from 'dvc/src/plots/webview/contract' -type CheckpointPlotsById = { [key: string]: CheckpointPlotData } +export type CheckpointPlotsById = { [key: string]: CheckpointPlotData } export const plotDataStore = { checkpoint: {} as CheckpointPlotsById, diff --git a/webview/src/plots/components/templatePlots/TemplatePlots.tsx b/webview/src/plots/components/templatePlots/TemplatePlots.tsx index 1e3d64bf63..19bee5366e 100644 --- a/webview/src/plots/components/templatePlots/TemplatePlots.tsx +++ b/webview/src/plots/components/templatePlots/TemplatePlots.tsx @@ -17,6 +17,7 @@ import { shouldUseVirtualizedGrid } from '../util' import { useNbItemsPerRow } from '../../hooks/useNbItemsPerRow' import { PlotsState } from '../../store' import { plotDataStore } from '../plotDataStore' +import { EmptyState } from '../../../shared/components/emptyState/EmptyState' export enum NewSectionBlock { TOP = 'drop-section-top', @@ -38,7 +39,7 @@ export const TemplatePlots: React.FC = () => { }, [plotsSnapshot, setSections]) useEffect(() => { - if (shouldSendMessage.current) { + if (sections && shouldSendMessage.current) { sendMessage({ payload: sections.map(section => ({ group: section.group, @@ -50,6 +51,10 @@ export const TemplatePlots: React.FC = () => { shouldSendMessage.current = true }, [sections]) + if (!sections || sections.length === 0) { + return No Plots to Display + } + const setSectionEntries = (index: number, entries: TemplatePlotEntry[]) => { setSections(sections => { const updatedSections = [...sections]