From b2f6d026da9b85786103641448ef44c180151f2d Mon Sep 17 00:00:00 2001 From: julieg18 Date: Mon, 18 Sep 2023 09:47:59 -0500 Subject: [PATCH 1/8] Merge create plot commands --- extension/package.json | 15 ++----- extension/src/commands/external.ts | 2 + extension/src/commands/util.ts | 39 +++++++++++++++++++ extension/src/extension.ts | 6 ++- extension/src/plots/webview/messages.ts | 8 ++-- extension/src/telemetry/constants.ts | 2 + extension/src/vscode/title.ts | 1 + extension/src/webview/contract.ts | 4 +- .../plots/components/emptyState/AddPlots.tsx | 19 +-------- .../components/emptyState/GetStarted.tsx | 5 +-- .../plots/components/emptyState/Welcome.tsx | 4 +- .../src/plots/components/ribbon/Ribbon.tsx | 11 +++++- .../templatePlots/TemplatePlotsWrapper.tsx | 2 - webview/src/plots/util/messages.ts | 9 ++--- 14 files changed, 77 insertions(+), 50 deletions(-) diff --git a/extension/package.json b/extension/package.json index bfcc946e36..c26f9bd5e8 100644 --- a/extension/package.json +++ b/extension/package.json @@ -425,7 +425,7 @@ }, { "title": "Add Plot", - "command": "dvc.addTopLevelPlot", + "command": "dvc.addPlot", "category": "DVC", "icon": "$(add)" }, @@ -601,11 +601,6 @@ "category": "DVC", "icon": "$(refresh)" }, - { - "title": "Add Custom Plot", - "command": "dvc.views.plots.addCustomPlot", - "category": "DVC" - }, { "title": "Remove Custom Plot(s)", "command": "dvc.views.plots.removeCustomPlots", @@ -910,7 +905,7 @@ "when": "dvc.commands.available && dvc.project.available" }, { - "command": "dvc.addTopLevelPlot", + "command": "dvc.addPlot", "when": "dvc.commands.available && dvc.project.available" }, { @@ -1029,10 +1024,6 @@ "command": "dvc.views.plots.refreshPlots", "when": "dvc.commands.available && dvc.project.available" }, - { - "command": "dvc.views.plots.addCustomPlot", - "when": "dvc.commands.available && dvc.project.available" - }, { "command": "dvc.views.plots.removeCustomPlots", "when": "dvc.commands.available && dvc.project.available" @@ -1425,7 +1416,7 @@ "group": "navigation@3" }, { - "command": "dvc.addTopLevelPlot", + "command": "dvc.addPlot", "when": "view == dvc.views.plotsPathsTree", "group": "navigation@0" }, diff --git a/extension/src/commands/external.ts b/extension/src/commands/external.ts index ebd61eff8e..1a9b19a451 100644 --- a/extension/src/commands/external.ts +++ b/extension/src/commands/external.ts @@ -81,6 +81,8 @@ export enum RegisteredCommands { PLOTS_CUSTOM_ADD = 'dvc.views.plots.addCustomPlot', PLOTS_CUSTOM_REMOVE = 'dvc.views.plots.removeCustomPlots', + ADD_PLOT = 'dvc.addPlot', + EXPERIMENT_AND_PLOTS_SHOW = 'dvc.showExperimentsAndPlots', EXTENSION_CHECK_CLI_COMPATIBLE = 'dvc.checkCLICompatible', diff --git a/extension/src/commands/util.ts b/extension/src/commands/util.ts index 906140598c..03d4171c52 100644 --- a/extension/src/commands/util.ts +++ b/extension/src/commands/util.ts @@ -2,6 +2,8 @@ import { commands } from 'vscode' import { RegisteredCommands } from './external' import { Setup } from '../setup' import { Context } from '../vscode/context' +import { quickPickValue } from '../vscode/quickPick' +import { Title } from '../vscode/title' export const showSetupOrExecuteCommand = (setup: Setup, callback: (context: Context) => Promise) => @@ -18,3 +20,40 @@ export const showSetupOrExecuteCommand = return callback(context) } + +const enum PLOT_TYPE { + CUSTOM, + TOP_LEVEL +} + +export const addPlotCommand = async (context: Context) => { + const result: PLOT_TYPE | undefined = await quickPickValue( + [ + { + description: 'Create a dvc.yaml plot based off a chosen data file', + label: 'Top-Level', + value: PLOT_TYPE.TOP_LEVEL + }, + { + description: + 'Create an extension-only plot based off a chosen metric and param', + label: 'Custom', + value: PLOT_TYPE.CUSTOM + } + ], + { + title: Title.SELECT_PLOT_TYPE + } + ) + + if (result === PLOT_TYPE.CUSTOM) { + return commands.executeCommand(RegisteredCommands.PLOTS_CUSTOM_ADD, context) + } + + if (result === PLOT_TYPE.TOP_LEVEL) { + return commands.executeCommand( + RegisteredCommands.PIPELINE_ADD_PLOT, + context + ) + } +} diff --git a/extension/src/extension.ts b/extension/src/extension.ts index b3cdef2a14..792d60b075 100644 --- a/extension/src/extension.ts +++ b/extension/src/extension.ts @@ -49,7 +49,7 @@ import { DvcViewer } from './cli/dvc/viewer' import { registerSetupCommands } from './setup/commands/register' import { Status } from './status' import { registerPersistenceCommands } from './persistence/register' -import { showSetupOrExecuteCommand } from './commands/util' +import { addPlotCommand, showSetupOrExecuteCommand } from './commands/util' import { WorkspacePipeline } from './pipeline/workspace' import { registerPipelineCommands } from './pipeline/register' @@ -193,6 +193,10 @@ class Extension extends Disposable { ) registerPipelineCommands(this.pipelines, this.internalCommands) registerPlotsCommands(this.plots, this.internalCommands, this.setup) + this.internalCommands.registerExternalCommand( + RegisteredCommands.ADD_PLOT, + addPlotCommand + ) registerSetupCommands(this.setup, this.internalCommands) this.internalCommands.registerExternalCommand( RegisteredCommands.EXPERIMENT_AND_PLOTS_SHOW, diff --git a/extension/src/plots/webview/messages.ts b/extension/src/plots/webview/messages.ts index d4b14e99a2..3e6da01432 100644 --- a/extension/src/plots/webview/messages.ts +++ b/extension/src/plots/webview/messages.ts @@ -71,14 +71,14 @@ export class WebviewMessages { public handleMessageFromWebview(message: MessageFromWebview) { switch (message.type) { - case MessageFromWebviewType.ADD_CUSTOM_PLOT: + case MessageFromWebviewType.ADD_PLOT: return commands.executeCommand( - RegisteredCommands.PLOTS_CUSTOM_ADD, + RegisteredCommands.ADD_PLOT, this.dvcRoot ) - case MessageFromWebviewType.ADD_PIPELINE_PLOT: + case MessageFromWebviewType.ADD_CUSTOM_PLOT: return commands.executeCommand( - RegisteredCommands.PIPELINE_ADD_PLOT, + RegisteredCommands.PLOTS_CUSTOM_ADD, this.dvcRoot ) case MessageFromWebviewType.EXPORT_PLOT_DATA_AS_CSV: diff --git a/extension/src/telemetry/constants.ts b/extension/src/telemetry/constants.ts index ff8a1c9a5f..5204a2fae2 100644 --- a/extension/src/telemetry/constants.ts +++ b/extension/src/telemetry/constants.ts @@ -204,6 +204,8 @@ export interface IEventNamePropertyMapping { [EventName.PLOTS_CUSTOM_ADD]: undefined [EventName.PLOTS_CUSTOM_REMOVE]: undefined + [EventName.ADD_PLOT]: undefined + [EventName.ADD_TARGET]: undefined [EventName.CHECKOUT_TARGET]: undefined [EventName.CHECKOUT]: undefined diff --git a/extension/src/vscode/title.ts b/extension/src/vscode/title.ts index b015c66e5c..d9fbbf486b 100644 --- a/extension/src/vscode/title.ts +++ b/extension/src/vscode/title.ts @@ -41,6 +41,7 @@ export enum Title { SELECT_SORTS_TO_REMOVE = 'Select Sort(s) to Remove', SELECT_TRAINING_SCRIPT = 'Select your Training Script', SELECT_PLOT_DATA = 'Select Data to Plot', + SELECT_PLOT_TYPE = 'Select Plot Type', SETUP_WORKSPACE = 'Setup the Workspace', SET_EXPERIMENTS_HEADER_HEIGHT = 'Set the Maximum Experiment Table Header Height', SET_REMOTE_AS_DEFAULT = 'Set Default Remote', diff --git a/extension/src/webview/contract.ts b/extension/src/webview/contract.ts index ed3708d586..5138717d3b 100644 --- a/extension/src/webview/contract.ts +++ b/extension/src/webview/contract.ts @@ -16,8 +16,8 @@ export enum MessageFromWebviewType { ADD_CONFIGURATION = 'add-configuration', APPLY_EXPERIMENT_TO_WORKSPACE = 'apply-experiment-to-workspace', ADD_STARRED_EXPERIMENT_FILTER = 'add-starred-experiment-filter', + ADD_PLOT = 'add-plot', ADD_CUSTOM_PLOT = 'add-custom-plot', - ADD_PIPELINE_PLOT = 'add-pipeline-plot', CREATE_BRANCH_FROM_EXPERIMENT = 'create-branch-from-experiment', COPY_TO_CLIPBOARD = 'copy-to-clipboard', COPY_STUDIO_LINK = 'copy-studio-link', @@ -112,7 +112,7 @@ export type MessageFromWebview = type: MessageFromWebviewType.ADD_CUSTOM_PLOT } | { - type: MessageFromWebviewType.ADD_PIPELINE_PLOT + type: MessageFromWebviewType.ADD_PLOT } | { type: MessageFromWebviewType.COPY_TO_CLIPBOARD diff --git a/webview/src/plots/components/emptyState/AddPlots.tsx b/webview/src/plots/components/emptyState/AddPlots.tsx index e162f59f7b..cbc401963c 100644 --- a/webview/src/plots/components/emptyState/AddPlots.tsx +++ b/webview/src/plots/components/emptyState/AddPlots.tsx @@ -1,19 +1,12 @@ import React from 'react' -import { - addCustomPlot, - addPipelinePlot, - selectPlots, - selectRevisions -} from '../../util/messages' +import { addPlot, selectPlots, selectRevisions } from '../../util/messages' import { StartButton } from '../../../shared/components/button/StartButton' type AddPlotsProps = { - hasCustomPlots: boolean hasUnselectedPlots: boolean } export const AddPlots: React.FC = ({ - hasCustomPlots, hasUnselectedPlots }: AddPlotsProps) => (
@@ -31,18 +24,10 @@ export const AddPlots: React.FC = ({ )} - {!hasCustomPlots && ( - - )}
) diff --git a/webview/src/plots/components/emptyState/GetStarted.tsx b/webview/src/plots/components/emptyState/GetStarted.tsx index 2cd70757e0..0f507ee3f4 100644 --- a/webview/src/plots/components/emptyState/GetStarted.tsx +++ b/webview/src/plots/components/emptyState/GetStarted.tsx @@ -18,10 +18,7 @@ export const GetStarted: React.FC = ({ }) => ( {hasPlots ? ( - + ) : ( )} diff --git a/webview/src/plots/components/emptyState/Welcome.tsx b/webview/src/plots/components/emptyState/Welcome.tsx index 52e8e3aa81..eb2cbfb196 100644 --- a/webview/src/plots/components/emptyState/Welcome.tsx +++ b/webview/src/plots/components/emptyState/Welcome.tsx @@ -1,12 +1,12 @@ import React from 'react' import { StartButton } from '../../../shared/components/button/StartButton' -import { addPipelinePlot, selectRevisions } from '../../util/messages' +import { addPlot, selectRevisions } from '../../util/messages' export const Welcome: React.FC = () => (

No Plots Detected.

- + { text={`${revisions.length} of ${MAX_NB_EXP}`} /> +
  • + +
  • { const { nbItemsPerRow, isCollapsed, height, hasItems } = useSelector( @@ -21,7 +20,6 @@ export const TemplatePlotsWrapper: React.FC = () => { changeSize={changeSize} hasItems={hasItems} height={height} - addPlotsButton={{ onClick: addPipelinePlot }} > diff --git a/webview/src/plots/util/messages.ts b/webview/src/plots/util/messages.ts index 506903eabc..0b5ac4ba31 100644 --- a/webview/src/plots/util/messages.ts +++ b/webview/src/plots/util/messages.ts @@ -3,16 +3,15 @@ import { PlotsSection } from 'dvc/src/plots/webview/contract' import { sendMessage } from '../../shared/vscode' import { PlotGroup } from '../components/templatePlots/templatePlotsSlice' -export const addCustomPlot = () => +export const addPlot = () => sendMessage({ - type: MessageFromWebviewType.ADD_CUSTOM_PLOT + type: MessageFromWebviewType.ADD_PLOT }) -export const addPipelinePlot = () => { +export const addCustomPlot = () => sendMessage({ - type: MessageFromWebviewType.ADD_PIPELINE_PLOT + type: MessageFromWebviewType.ADD_CUSTOM_PLOT }) -} export const exportPlotDataAsCsv = (id: string) => { sendMessage({ From d7d3f5fdf33fada75d4b2d32e5470b78c709ec5b Mon Sep 17 00:00:00 2001 From: julieg18 Date: Mon, 18 Sep 2023 10:58:30 -0500 Subject: [PATCH 2/8] Add tests --- extension/src/commands/util.test.ts | 66 ++++++++++++++++ extension/src/commands/util.ts | 4 +- extension/src/test/suite/plots/index.test.ts | 6 +- webview/src/plots/components/App.test.tsx | 75 ++++++++++++------- .../plots/components/emptyState/AddPlots.tsx | 15 ++-- 5 files changed, 125 insertions(+), 41 deletions(-) create mode 100644 extension/src/commands/util.test.ts diff --git a/extension/src/commands/util.test.ts b/extension/src/commands/util.test.ts new file mode 100644 index 0000000000..945df09e85 --- /dev/null +++ b/extension/src/commands/util.test.ts @@ -0,0 +1,66 @@ +import { commands } from 'vscode' +import { RegisteredCommands } from './external' +import { addPlotCommand } from './util' +import { quickPickValue } from '../vscode/quickPick' +import { Title } from '../vscode/title' + +jest.mock('../vscode/quickPick') + +const mockedQuickPickValue = jest.mocked(quickPickValue) +const mockedCommands = jest.mocked(commands) +const mockedExecuteCommand = jest.fn() +mockedCommands.executeCommand = mockedExecuteCommand + +beforeEach(() => { + jest.resetAllMocks() +}) + +describe('addPlotCommand', () => { + it('should call no add commands if no plots types are selected', async () => { + mockedQuickPickValue.mockResolvedValueOnce(undefined) + + await addPlotCommand(undefined) + + expect(mockedQuickPickValue).toHaveBeenCalledWith( + [ + { + description: 'Create a dvc.yaml plot based off a chosen data file', + label: 'Top-Level', + value: 'top-level' + }, + { + description: + 'Create an extension-only plot based off a chosen metric and param', + label: 'Custom', + value: 'custom' + } + ], + { + title: Title.SELECT_PLOT_TYPE + } + ) + expect(mockedExecuteCommand).not.toHaveBeenCalled() + }) + + it('should call the add top-level plot command if the top-level type is selected', async () => { + mockedQuickPickValue.mockResolvedValueOnce('top-level') + + await addPlotCommand(undefined) + + expect(mockedExecuteCommand).toHaveBeenCalledWith( + RegisteredCommands.PIPELINE_ADD_PLOT, + undefined + ) + }) + + it('should call the add custom plot command if the custom type is selected', async () => { + mockedQuickPickValue.mockResolvedValueOnce('custom') + + await addPlotCommand('cwd') + + expect(mockedExecuteCommand).toHaveBeenCalledWith( + RegisteredCommands.PLOTS_CUSTOM_ADD, + 'cwd' + ) + }) +}) diff --git a/extension/src/commands/util.ts b/extension/src/commands/util.ts index 03d4171c52..c252237481 100644 --- a/extension/src/commands/util.ts +++ b/extension/src/commands/util.ts @@ -22,8 +22,8 @@ export const showSetupOrExecuteCommand = } const enum PLOT_TYPE { - CUSTOM, - TOP_LEVEL + CUSTOM = 'custom', + TOP_LEVEL = 'top-level' } export const addPlotCommand = async (context: Context) => { diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index 699a4414e6..8176754299 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -1209,7 +1209,7 @@ suite('Plots Test Suite', () => { ) }) - it('should handle an add pipeline plot message from the webview', async () => { + it('should handle an add plot message from the webview', async () => { const { mockMessageReceived } = await buildPlotsWebview({ disposer: disposable, plotsDiff: plotsDiffFixture @@ -1218,11 +1218,11 @@ suite('Plots Test Suite', () => { const executeCommandSpy = spy(commands, 'executeCommand') mockMessageReceived.fire({ - type: MessageFromWebviewType.ADD_PIPELINE_PLOT + type: MessageFromWebviewType.ADD_PLOT }) expect(executeCommandSpy).to.be.calledWithExactly( - RegisteredCommands.PIPELINE_ADD_PLOT, + RegisteredCommands.ADD_PLOT, dvcDemoPath ) }) diff --git a/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx index 356e3ee8da..ffb2bec82e 100644 --- a/webview/src/plots/components/App.test.tsx +++ b/webview/src/plots/components/App.test.tsx @@ -336,7 +336,7 @@ describe('App', () => { expect(loading).toHaveLength(3) }) - it('should render only get started (buttons: select plots, add experiments, add custom plots) when there are some selected exps, all unselected plots, and no custom plots', async () => { + it('should render only get started (buttons: select plots, add experiments, add plot) when there are some selected exps, all unselected plots, and no custom plots', async () => { renderAppWithOptionalData({ hasPlots: true, hasUnselectedPlots: true, @@ -344,11 +344,11 @@ describe('App', () => { }) const addExperimentsButton = await screen.findByText('Add Experiments') const selectPlotsButton = await screen.findByText('Select Plots') - const addCustomPlotsButton = await screen.findByText('Add Custom Plot') + const addPlotsButton = await screen.findByText('Add Plot') expect(addExperimentsButton).toBeInTheDocument() expect(selectPlotsButton).toBeInTheDocument() - expect(addCustomPlotsButton).toBeInTheDocument() + expect(addPlotsButton).toBeInTheDocument() expect(screen.queryByTestId('section-container')).not.toBeInTheDocument() mockPostMessage.mockReset() @@ -368,10 +368,10 @@ describe('App', () => { }) mockPostMessage.mockReset() - fireEvent.click(addCustomPlotsButton) + fireEvent.click(addPlotsButton) expect(mockPostMessage).toHaveBeenCalledWith({ - type: MessageFromWebviewType.ADD_CUSTOM_PLOT + type: MessageFromWebviewType.ADD_PLOT }) mockPostMessage.mockReset() }) @@ -391,7 +391,7 @@ describe('App', () => { const refreshButton = await screen.findByText('Refresh') expect(addExperimentsButton).toBeInTheDocument() - expect(screen.queryByText('Add Plots')).not.toBeInTheDocument() + expect(screen.queryByText('Add Plot')).not.toBeInTheDocument() expect(addCustomPlotsButton).toBeInTheDocument() expect(errorIcon).toBeInTheDocument() expect(screen.queryByTestId('section-container')).not.toBeInTheDocument() @@ -427,7 +427,7 @@ describe('App', () => { mockPostMessage.mockReset() }) - it('should render get started (buttons: select plots, add experiments) and custom section when there are some selected exps, all unselected plots, and added custom plots', async () => { + it('should render get started (buttons: select plots, add experiments, add plot) and custom section when there are some selected exps, all unselected plots, and added custom plots', async () => { renderAppWithOptionalData({ custom: customPlotsFixture, hasPlots: true, @@ -436,12 +436,12 @@ describe('App', () => { }) const addExperimentsButton = await screen.findByText('Add Experiments') const selectPlotsButton = await screen.findByText('Select Plots') - const addCustomPlotsButton = screen.queryByText('Add Custom Plot') + const addPlotButton = await screen.findByText('Add Plot') const customSection = await screen.findByTestId('section-container') expect(addExperimentsButton).toBeInTheDocument() expect(selectPlotsButton).toBeInTheDocument() - expect(addCustomPlotsButton).not.toBeInTheDocument() + expect(addPlotButton).toBeInTheDocument() expect(customSection).toBeInTheDocument() mockPostMessage.mockReset() @@ -460,9 +460,15 @@ describe('App', () => { type: MessageFromWebviewType.SELECT_PLOTS }) mockPostMessage.mockReset() + + fireEvent.click(addPlotButton) + + expect(mockPostMessage).toHaveBeenCalledWith({ + type: MessageFromWebviewType.ADD_PLOT + }) }) - it('should render only get started (buttons: add experiments, add custom plots, add plots) when there are no selected exps and no custom plots', async () => { + it('should render only get started (buttons: add experiments, add plot) when there are no selected exps and no custom plots', async () => { renderAppWithOptionalData({ custom: null, hasPlots: true, @@ -470,13 +476,11 @@ describe('App', () => { selectedRevisions: undefined }) const addExperimentsButton = await screen.findByText('Add Experiments') - const addPlotsButton = await screen.findByText('Add Plot') - const addCustomPlotsButton = await screen.findByText('Add Custom Plot') + const addPlotButton = await screen.findByText('Add Plot') const customSection = screen.queryByTestId('section-container') expect(addExperimentsButton).toBeInTheDocument() - expect(addCustomPlotsButton).toBeInTheDocument() - expect(addPlotsButton).toBeInTheDocument() + expect(addPlotButton).toBeInTheDocument() expect(customSection).not.toBeInTheDocument() mockPostMessage.mockReset() @@ -486,18 +490,12 @@ describe('App', () => { type: MessageFromWebviewType.SELECT_EXPERIMENTS }) - fireEvent.click(addCustomPlotsButton) + fireEvent.click(addPlotButton) expect(mockPostMessage).toHaveBeenCalledWith({ - type: MessageFromWebviewType.ADD_CUSTOM_PLOT + type: MessageFromWebviewType.ADD_PLOT }) mockPostMessage.mockReset() - - fireEvent.click(addPlotsButton) - - expect(mockPostMessage).toHaveBeenCalledWith({ - type: MessageFromWebviewType.ADD_PIPELINE_PLOT - }) }) it('should render get started (buttons: add experiments, add plots) and custom section when there are no selected exps and added custom plots', async () => { @@ -509,11 +507,9 @@ describe('App', () => { }) const addExperimentsButton = await screen.findByText('Add Experiments') const addPlotsButton = await screen.findByText('Add Plot') - const addCustomPlotsButton = screen.queryByText('Add Custom Plot') const customSection = await screen.findByTestId('section-container') expect(addExperimentsButton).toBeInTheDocument() - expect(addCustomPlotsButton).not.toBeInTheDocument() expect(addPlotsButton).toBeInTheDocument() expect(customSection).toBeInTheDocument() @@ -528,7 +524,7 @@ describe('App', () => { fireEvent.click(addPlotsButton) expect(mockPostMessage).toHaveBeenCalledWith({ - type: MessageFromWebviewType.ADD_PIPELINE_PLOT + type: MessageFromWebviewType.ADD_PLOT }) }) @@ -2362,7 +2358,12 @@ describe('App', () => { const revisionBlocks = within(ribbon).getAllByRole('listitem') return revisionBlocks .map(item => item.textContent) - .filter(text => !text?.includes(' of ') && text !== 'Refresh All') + .filter( + text => + !text?.includes(' of ') && + text !== 'Refresh All' && + text !== 'Add Plot' + ) } it('should show the revisions at the top', () => { @@ -2427,16 +2428,34 @@ describe('App', () => { }) }) - it('should send a message to refresh each revision when clicking the refresh all button', () => { + it('should send a message to add a plot when clicking the add plot button', () => { renderAppWithOptionalData({ comparison: comparisonTableFixture, + selectedRevisions: plotsRevisionsFixture + }) + + const addPlotButton = within(screen.getByTestId('ribbon')).getAllByRole( + 'button' + )[1] + + mockPostMessage.mockReset() + fireEvent.click(addPlotButton) + expect(mockPostMessage).toHaveBeenCalledTimes(1) + expect(mockPostMessage).toHaveBeenCalledWith({ + type: MessageFromWebviewType.ADD_PLOT + }) + }) + + it('should send a message to refresh each revision when clicking the refresh all button', () => { + renderAppWithOptionalData({ + comparison: comparisonTableFixture, selectedRevisions: plotsRevisionsFixture }) const refreshAllButton = within( screen.getByTestId('ribbon') - ).getAllByRole('button')[1] + ).getAllByRole('button')[2] mockPostMessage.mockReset() fireEvent.click(refreshAllButton) diff --git a/webview/src/plots/components/emptyState/AddPlots.tsx b/webview/src/plots/components/emptyState/AddPlots.tsx index cbc401963c..5ddcc8b2f1 100644 --- a/webview/src/plots/components/emptyState/AddPlots.tsx +++ b/webview/src/plots/components/emptyState/AddPlots.tsx @@ -13,21 +13,20 @@ export const AddPlots: React.FC = ({

    No Plots to Display

    - {hasUnselectedPlots ? ( + {hasUnselectedPlots && ( - ) : ( - )} +
    • ) From 955edf557d6e16950555e38e057219bb91e4cef7 Mon Sep 17 00:00:00 2001 From: julieg18 Date: Mon, 18 Sep 2023 11:23:21 -0500 Subject: [PATCH 3/8] Fix e2e tests --- extension/src/test/e2e/util.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/extension/src/test/e2e/util.ts b/extension/src/test/e2e/util.ts index dd67c309b4..ed37beaf82 100644 --- a/extension/src/test/e2e/util.ts +++ b/extension/src/test/e2e/util.ts @@ -185,7 +185,9 @@ export const closeAllEditors = async (): Promise => { export const createCustomPlot = async (): Promise => { const workbench = await browser.getWorkbench() - const addCustomPlot = await workbench.executeCommand('DVC: Add Custom Plot') + const addCustomPlot = await workbench.executeCommand('DVC: Add Plot') + await browser.waitUntil(() => addCustomPlot.elem.isDisplayed()) + await browser.keys(['ArrowDown', 'Enter']) await browser.waitUntil(() => addCustomPlot.elem.isDisplayed()) await browser.keys('Enter') await browser.waitUntil(() => addCustomPlot.elem.isDisplayed()) From 93112233324cf95593f77a49bd3c70563777525d Mon Sep 17 00:00:00 2001 From: julieg18 Date: Tue, 19 Sep 2023 10:01:52 -0500 Subject: [PATCH 4/8] resolve comments --- extension/src/commands/external.ts | 2 - extension/src/commands/util.test.ts | 66 ------------------- extension/src/commands/util.ts | 39 ----------- extension/src/extension.ts | 11 ++-- extension/src/pipeline/register.ts | 7 -- extension/src/plots/commands/register.ts | 14 ++-- extension/src/plots/index.ts | 3 +- extension/src/plots/quickPick.test.ts | 39 +++++++++++ extension/src/plots/quickPick.ts | 28 ++++++++ extension/src/plots/webview/messages.ts | 15 +++-- extension/src/plots/workspace.ts | 24 +++++++ extension/src/telemetry/constants.ts | 6 +- extension/src/test/suite/plots/index.test.ts | 13 ++-- .../src/test/suite/plots/workspace.test.ts | 49 ++++++++++++-- webview/src/plots/components/App.test.tsx | 24 +++---- .../src/plots/components/ribbon/Ribbon.tsx | 10 +-- 16 files changed, 188 insertions(+), 162 deletions(-) delete mode 100644 extension/src/commands/util.test.ts create mode 100644 extension/src/plots/quickPick.test.ts create mode 100644 extension/src/plots/quickPick.ts diff --git a/extension/src/commands/external.ts b/extension/src/commands/external.ts index 1a9b19a451..e4e6eeb304 100644 --- a/extension/src/commands/external.ts +++ b/extension/src/commands/external.ts @@ -72,13 +72,11 @@ export enum RegisteredCommands { STOP_EXPERIMENTS = 'dvc.stopAllRunningExperiments', PIPELINE_SHOW_DAG = 'dvc.showPipelineDAG', - PIPELINE_ADD_PLOT = 'dvc.addTopLevelPlot', PLOTS_PATH_TOGGLE = 'dvc.views.plotsPathsTree.toggleStatus', PLOTS_SHOW = 'dvc.showPlots', PLOTS_SELECT = 'dvc.views.plotsPathsTree.selectPlots', PLOTS_REFRESH = 'dvc.views.plots.refreshPlots', - PLOTS_CUSTOM_ADD = 'dvc.views.plots.addCustomPlot', PLOTS_CUSTOM_REMOVE = 'dvc.views.plots.removeCustomPlots', ADD_PLOT = 'dvc.addPlot', diff --git a/extension/src/commands/util.test.ts b/extension/src/commands/util.test.ts deleted file mode 100644 index 945df09e85..0000000000 --- a/extension/src/commands/util.test.ts +++ /dev/null @@ -1,66 +0,0 @@ -import { commands } from 'vscode' -import { RegisteredCommands } from './external' -import { addPlotCommand } from './util' -import { quickPickValue } from '../vscode/quickPick' -import { Title } from '../vscode/title' - -jest.mock('../vscode/quickPick') - -const mockedQuickPickValue = jest.mocked(quickPickValue) -const mockedCommands = jest.mocked(commands) -const mockedExecuteCommand = jest.fn() -mockedCommands.executeCommand = mockedExecuteCommand - -beforeEach(() => { - jest.resetAllMocks() -}) - -describe('addPlotCommand', () => { - it('should call no add commands if no plots types are selected', async () => { - mockedQuickPickValue.mockResolvedValueOnce(undefined) - - await addPlotCommand(undefined) - - expect(mockedQuickPickValue).toHaveBeenCalledWith( - [ - { - description: 'Create a dvc.yaml plot based off a chosen data file', - label: 'Top-Level', - value: 'top-level' - }, - { - description: - 'Create an extension-only plot based off a chosen metric and param', - label: 'Custom', - value: 'custom' - } - ], - { - title: Title.SELECT_PLOT_TYPE - } - ) - expect(mockedExecuteCommand).not.toHaveBeenCalled() - }) - - it('should call the add top-level plot command if the top-level type is selected', async () => { - mockedQuickPickValue.mockResolvedValueOnce('top-level') - - await addPlotCommand(undefined) - - expect(mockedExecuteCommand).toHaveBeenCalledWith( - RegisteredCommands.PIPELINE_ADD_PLOT, - undefined - ) - }) - - it('should call the add custom plot command if the custom type is selected', async () => { - mockedQuickPickValue.mockResolvedValueOnce('custom') - - await addPlotCommand('cwd') - - expect(mockedExecuteCommand).toHaveBeenCalledWith( - RegisteredCommands.PLOTS_CUSTOM_ADD, - 'cwd' - ) - }) -}) diff --git a/extension/src/commands/util.ts b/extension/src/commands/util.ts index c252237481..906140598c 100644 --- a/extension/src/commands/util.ts +++ b/extension/src/commands/util.ts @@ -2,8 +2,6 @@ import { commands } from 'vscode' import { RegisteredCommands } from './external' import { Setup } from '../setup' import { Context } from '../vscode/context' -import { quickPickValue } from '../vscode/quickPick' -import { Title } from '../vscode/title' export const showSetupOrExecuteCommand = (setup: Setup, callback: (context: Context) => Promise) => @@ -20,40 +18,3 @@ export const showSetupOrExecuteCommand = return callback(context) } - -const enum PLOT_TYPE { - CUSTOM = 'custom', - TOP_LEVEL = 'top-level' -} - -export const addPlotCommand = async (context: Context) => { - const result: PLOT_TYPE | undefined = await quickPickValue( - [ - { - description: 'Create a dvc.yaml plot based off a chosen data file', - label: 'Top-Level', - value: PLOT_TYPE.TOP_LEVEL - }, - { - description: - 'Create an extension-only plot based off a chosen metric and param', - label: 'Custom', - value: PLOT_TYPE.CUSTOM - } - ], - { - title: Title.SELECT_PLOT_TYPE - } - ) - - if (result === PLOT_TYPE.CUSTOM) { - return commands.executeCommand(RegisteredCommands.PLOTS_CUSTOM_ADD, context) - } - - if (result === PLOT_TYPE.TOP_LEVEL) { - return commands.executeCommand( - RegisteredCommands.PIPELINE_ADD_PLOT, - context - ) - } -} diff --git a/extension/src/extension.ts b/extension/src/extension.ts index 792d60b075..24e571619b 100644 --- a/extension/src/extension.ts +++ b/extension/src/extension.ts @@ -49,7 +49,7 @@ import { DvcViewer } from './cli/dvc/viewer' import { registerSetupCommands } from './setup/commands/register' import { Status } from './status' import { registerPersistenceCommands } from './persistence/register' -import { addPlotCommand, showSetupOrExecuteCommand } from './commands/util' +import { showSetupOrExecuteCommand } from './commands/util' import { WorkspacePipeline } from './pipeline/workspace' import { registerPipelineCommands } from './pipeline/register' @@ -192,10 +192,11 @@ class Extension extends Disposable { this.setup ) registerPipelineCommands(this.pipelines, this.internalCommands) - registerPlotsCommands(this.plots, this.internalCommands, this.setup) - this.internalCommands.registerExternalCommand( - RegisteredCommands.ADD_PLOT, - addPlotCommand + registerPlotsCommands( + this.plots, + this.internalCommands, + this.setup, + this.pipelines ) registerSetupCommands(this.setup, this.internalCommands) this.internalCommands.registerExternalCommand( diff --git a/extension/src/pipeline/register.ts b/extension/src/pipeline/register.ts index 468db70301..75b9580e28 100644 --- a/extension/src/pipeline/register.ts +++ b/extension/src/pipeline/register.ts @@ -1,7 +1,6 @@ import { WorkspacePipeline } from './workspace' import { RegisteredCommands } from '../commands/external' import { InternalCommands } from '../commands/internal' -import { Context, getDvcRootFromContext } from '../vscode/context' export const registerPipelineCommands = ( pipelines: WorkspacePipeline, @@ -11,10 +10,4 @@ export const registerPipelineCommands = ( RegisteredCommands.PIPELINE_SHOW_DAG, () => pipelines.showDag() ) - - internalCommands.registerExternalCommand( - RegisteredCommands.PIPELINE_ADD_PLOT, - (context: Context) => - pipelines.addTopLevelPlot(getDvcRootFromContext(context)) - ) } diff --git a/extension/src/plots/commands/register.ts b/extension/src/plots/commands/register.ts index beed77877e..f271cb7de0 100644 --- a/extension/src/plots/commands/register.ts +++ b/extension/src/plots/commands/register.ts @@ -1,15 +1,22 @@ +import { commands } from 'vscode' import { RegisteredCommands } from '../../commands/external' import { InternalCommands } from '../../commands/internal' import { showSetupOrExecuteCommand } from '../../commands/util' import { Setup } from '../../setup' import { Context, getDvcRootFromContext } from '../../vscode/context' import { WorkspacePlots } from '../workspace' +import { WorkspacePipeline } from '../../pipeline/workspace' export const registerPlotsCommands = ( plots: WorkspacePlots, internalCommands: InternalCommands, - setup: Setup + setup: Setup, + pipelines: WorkspacePipeline ) => { + commands.registerCommand(RegisteredCommands.ADD_PLOT, (context: Context) => + plots.addPlot(pipelines, getDvcRootFromContext(context)) + ) + internalCommands.registerExternalCommand( RegisteredCommands.PLOTS_SHOW, showSetupOrExecuteCommand(setup, context => @@ -27,11 +34,6 @@ export const registerPlotsCommands = ( (context: Context) => plots.refresh(getDvcRootFromContext(context)) ) - internalCommands.registerExternalCommand( - RegisteredCommands.PLOTS_CUSTOM_ADD, - (context: Context) => plots.addCustomPlot(getDvcRootFromContext(context)) - ) - internalCommands.registerExternalCommand( RegisteredCommands.PLOTS_CUSTOM_REMOVE, (context: Context) => diff --git a/extension/src/plots/index.ts b/extension/src/plots/index.ts index 41c31ff5a6..ae7ac0ddc7 100644 --- a/extension/src/plots/index.ts +++ b/extension/src/plots/index.ts @@ -223,7 +223,8 @@ export class Plots extends BaseRepository { errors, experiments, () => this.getWebview(), - () => this.selectPlots() + () => this.selectPlots(), + () => this.addCustomPlot() ) this.dispose.track( this.onDidReceivedWebviewMessage(message => diff --git a/extension/src/plots/quickPick.test.ts b/extension/src/plots/quickPick.test.ts new file mode 100644 index 0000000000..b3ee56c8f0 --- /dev/null +++ b/extension/src/plots/quickPick.test.ts @@ -0,0 +1,39 @@ +import { pickPlotType } from './quickPick' +import { quickPickValue } from '../vscode/quickPick' +import { Title } from '../vscode/title' + +jest.mock('../vscode/quickPick') + +const mockedQuickPickValue = jest.mocked(quickPickValue) + +beforeEach(() => { + jest.resetAllMocks() +}) + +describe('pickPlotType', () => { + it('should call a quick pick with possible plot types', async () => { + mockedQuickPickValue.mockResolvedValueOnce(undefined) + + await pickPlotType() + + expect(mockedQuickPickValue).toHaveBeenCalledWith( + [ + { + description: + 'Create a top-level plot definition by selecting data from a file', + label: 'Top-Level', + value: 'top-level' + }, + { + description: + 'Create an extension-only plot by selecting one metric and one param from the experiments table', + label: 'Custom', + value: 'custom' + } + ], + { + title: Title.SELECT_PLOT_TYPE + } + ) + }) +}) diff --git a/extension/src/plots/quickPick.ts b/extension/src/plots/quickPick.ts new file mode 100644 index 0000000000..a37614236c --- /dev/null +++ b/extension/src/plots/quickPick.ts @@ -0,0 +1,28 @@ +import { quickPickValue } from '../vscode/quickPick' +import { Title } from '../vscode/title' + +export const enum PLOT_TYPE { + CUSTOM = 'custom', + TOP_LEVEL = 'top-level' +} + +export const pickPlotType = () => + quickPickValue( + [ + { + description: + 'Create a top-level plot definition by selecting data from a file', + label: 'Top-Level', + value: PLOT_TYPE.TOP_LEVEL + }, + { + description: + 'Create an extension-only plot by selecting one metric and one param from the experiments table', + label: 'Custom', + value: PLOT_TYPE.CUSTOM + } + ], + { + title: Title.SELECT_PLOT_TYPE + } + ) diff --git a/extension/src/plots/webview/messages.ts b/extension/src/plots/webview/messages.ts index 3e6da01432..dddc510cc2 100644 --- a/extension/src/plots/webview/messages.ts +++ b/extension/src/plots/webview/messages.ts @@ -41,6 +41,7 @@ export class WebviewMessages { private readonly getWebview: () => BaseWebview | undefined private readonly selectPlots: () => Promise + private readonly addCustomPlot: () => Promise constructor( dvcRoot: string, @@ -49,7 +50,8 @@ export class WebviewMessages { errors: ErrorsModel, experiments: Experiments, getWebview: () => BaseWebview | undefined, - selectPlots: () => Promise + selectPlots: () => Promise, + addCustomPlot: () => Promise ) { this.dvcRoot = dvcRoot this.paths = paths @@ -58,6 +60,7 @@ export class WebviewMessages { this.experiments = experiments this.getWebview = getWebview this.selectPlots = selectPlots + this.addCustomPlot = addCustomPlot } public async sendWebviewMessage() { @@ -77,10 +80,7 @@ export class WebviewMessages { this.dvcRoot ) case MessageFromWebviewType.ADD_CUSTOM_PLOT: - return commands.executeCommand( - RegisteredCommands.PLOTS_CUSTOM_ADD, - this.dvcRoot - ) + return this.addCustomPlotFromWebview() case MessageFromWebviewType.EXPORT_PLOT_DATA_AS_CSV: return this.exportPlotDataAsCsv(message.payload) case MessageFromWebviewType.EXPORT_PLOT_DATA_AS_TSV: @@ -309,6 +309,11 @@ export class WebviewMessages { ) } + private addCustomPlotFromWebview() { + void this.addCustomPlot() + sendTelemetryEvent(EventName.VIEWS_PLOTS_CUSTOM_ADD, undefined, undefined) + } + private setExperimentStatus(id: string) { this.experiments.toggleExperimentStatus(id) sendTelemetryEvent( diff --git a/extension/src/plots/workspace.ts b/extension/src/plots/workspace.ts index 296b10cbd9..008737748a 100644 --- a/extension/src/plots/workspace.ts +++ b/extension/src/plots/workspace.ts @@ -1,9 +1,14 @@ import { EventEmitter } from 'vscode' import { Plots } from '.' import { PlotsData } from './webview/contract' +import { PLOT_TYPE, pickPlotType } from './quickPick' import { ResourceLocator } from '../resourceLocator' import { BaseWorkspaceWebviews } from '../webview/workspace' import { WorkspaceExperiments } from '../experiments/workspace' +import { WorkspacePipeline } from '../pipeline/workspace' +import { sendTelemetryEvent } from '../telemetry' +import { RegisteredCommands } from '../commands/external' +import { StopWatch } from '../util/time' export class WorkspacePlots extends BaseWorkspaceWebviews { public readonly pathsChanged = this.dispose.track(new EventEmitter()) @@ -74,6 +79,25 @@ export class WorkspacePlots extends BaseWorkspaceWebviews { return this.getRepository(dvcRoot).removeCustomPlot() } + public async addPlot(pipelines: WorkspacePipeline, overrideRoot?: string) { + const stopWatch = new StopWatch() + const response: PLOT_TYPE | undefined = await pickPlotType() + + if (response === PLOT_TYPE.CUSTOM) { + await this.addCustomPlot(overrideRoot) + } + + if (response === PLOT_TYPE.TOP_LEVEL) { + await pipelines.addTopLevelPlot(overrideRoot) + } + + sendTelemetryEvent( + RegisteredCommands.ADD_PLOT, + { type: response }, + { duration: stopWatch.getElapsedTime() } + ) + } + public getFocusedOrOnlyOrPickProject() { return this.focusedWebviewDvcRoot || this.getOnlyOrPickProject() } diff --git a/extension/src/telemetry/constants.ts b/extension/src/telemetry/constants.ts index 5204a2fae2..bc5d629026 100644 --- a/extension/src/telemetry/constants.ts +++ b/extension/src/telemetry/constants.ts @@ -86,6 +86,7 @@ export const EventName = Object.assign( VIEWS_PLOTS_COMPARISON_ROWS_REORDERED: 'views.plots.comparisonRowsReordered', VIEWS_PLOTS_CREATED: 'views.plots.created', + VIEWS_PLOTS_CUSTOM_ADD: 'views.plots.custom', VIEWS_PLOTS_EXPERIMENT_TOGGLE: 'views.plots.toggleExperimentStatus', VIEWS_PLOTS_EXPORT_PLOT_DATA_AS_CSV: 'views.plots.exportPlotDataAsCsv', VIEWS_PLOTS_EXPORT_PLOT_DATA_AS_JSON: 'views.plots.exportPlotDataAsJson', @@ -195,16 +196,14 @@ export interface IEventNamePropertyMapping { [EventName.STOP_EXPERIMENTS]: { stopped: boolean; wasRunning: boolean } [EventName.PIPELINE_SHOW_DAG]: undefined - [EventName.PIPELINE_ADD_PLOT]: undefined [EventName.PLOTS_PATH_TOGGLE]: undefined [EventName.PLOTS_SHOW]: undefined [EventName.PLOTS_SELECT]: undefined [EventName.PLOTS_REFRESH]: undefined - [EventName.PLOTS_CUSTOM_ADD]: undefined [EventName.PLOTS_CUSTOM_REMOVE]: undefined - [EventName.ADD_PLOT]: undefined + [EventName.ADD_PLOT]: { type: string | undefined } [EventName.ADD_TARGET]: undefined [EventName.CHECKOUT_TARGET]: undefined @@ -289,6 +288,7 @@ export interface IEventNamePropertyMapping { [EventName.VIEWS_PLOTS_CLOSED]: undefined [EventName.VIEWS_PLOTS_CREATED]: undefined + [EventName.VIEWS_PLOTS_CUSTOM_ADD]: undefined [EventName.VIEWS_PLOTS_FOCUS_CHANGED]: WebviewFocusChangedProperties [EventName.VIEWS_PLOTS_REVISIONS_REORDERED]: undefined [EventName.VIEWS_PLOTS_COMPARISON_ROWS_REORDERED]: undefined diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index 8176754299..e1108e4d39 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -1107,20 +1107,23 @@ suite('Plots Test Suite', () => { }).timeout(WEBVIEW_TEST_TIMEOUT) it('should handle an add custom plot message from the webview', async () => { - const { mockMessageReceived } = await buildPlotsWebview({ + const { mockMessageReceived, plots } = await buildPlotsWebview({ disposer: disposable, plotsDiff: plotsDiffFixture }) - const executeCommandSpy = spy(commands, 'executeCommand') + const mockAddCustomPlot = stub(plots, 'addCustomPlot') + const mockSendTelemetryEvent = stub(Telemetry, 'sendTelemetryEvent') mockMessageReceived.fire({ type: MessageFromWebviewType.ADD_CUSTOM_PLOT }) - expect(executeCommandSpy).to.be.calledWithExactly( - RegisteredCommands.PLOTS_CUSTOM_ADD, - dvcDemoPath + expect(mockAddCustomPlot).to.be.calledOnce + expect(mockSendTelemetryEvent).to.be.calledWithExactly( + EventName.VIEWS_PLOTS_CUSTOM_ADD, + undefined, + undefined ) }) diff --git a/extension/src/test/suite/plots/workspace.test.ts b/extension/src/test/suite/plots/workspace.test.ts index b2219923db..f1a339044c 100644 --- a/extension/src/test/suite/plots/workspace.test.ts +++ b/extension/src/test/suite/plots/workspace.test.ts @@ -9,6 +9,8 @@ import { closeAllEditors } from '../util' import * as customPlotQuickPickUtil from '../../../plots/model/quickPick' import { customPlotsOrderFixture } from '../../fixtures/expShow/base/customPlots' import { RegisteredCommands } from '../../../commands/external' +import * as quickPickUtil from '../../../plots/quickPick' +import { WorkspacePipeline } from '../../../pipeline/workspace' suite('Workspace Plots Test Suite', () => { const disposable = Disposable.fn() @@ -23,13 +25,30 @@ suite('Workspace Plots Test Suite', () => { return closeAllEditors() }) - describe('dvc.views.plots.addCustomPlot', () => { + describe('dvc.addPlot', () => { + it('should be able to add a top-level plot', async () => { + const mockPickPlotType = stub(quickPickUtil, 'pickPlotType') + const mockAddTopLevelPlot = stub( + WorkspacePipeline.prototype, + 'addTopLevelPlot' + ) + + mockPickPlotType.onFirstCall().resolves(quickPickUtil.PLOT_TYPE.TOP_LEVEL) + mockAddTopLevelPlot.onFirstCall().resolves(undefined) + + await commands.executeCommand(RegisteredCommands.ADD_PLOT) + + expect(mockAddTopLevelPlot).to.be.calledOnce + }) + it('should be able to add a custom plot', async () => { const { plotsModel } = await buildPlots({ disposer: disposable, plotsDiff: plotsDiffFixture }) + const mockPickPlotType = stub(quickPickUtil, 'pickPlotType') + const mockGetMetricAndParam = stub( customPlotQuickPickUtil, 'pickMetricAndParam' @@ -40,12 +59,13 @@ suite('Workspace Plots Test Suite', () => { param: 'params.yaml:log_file' } + mockPickPlotType.onFirstCall().resolves(quickPickUtil.PLOT_TYPE.CUSTOM) mockGetMetricAndParam.onFirstCall().resolves(mockMetricVsParamOrderValue) const mockSetCustomPlotsOrder = stub(plotsModel, 'setCustomPlotsOrder') mockSetCustomPlotsOrder.returns(undefined) - await commands.executeCommand(RegisteredCommands.PLOTS_CUSTOM_ADD) + await commands.executeCommand(RegisteredCommands.ADD_PLOT) expect(mockSetCustomPlotsOrder).to.be.calledWith([ ...customPlotsOrderFixture, @@ -53,7 +73,26 @@ suite('Workspace Plots Test Suite', () => { ]) }) - it('should not add a custom plot if user cancels', async () => { + it('should not add a plot if users cancels', async () => { + const mockPickPlotType = stub(quickPickUtil, 'pickPlotType') + const mockAddTopLevelPlot = stub( + WorkspacePipeline.prototype, + 'addTopLevelPlot' + ) + const mockGetMetricAndParam = stub( + customPlotQuickPickUtil, + 'pickMetricAndParam' + ) + + mockPickPlotType.onFirstCall().resolves(undefined) + + await commands.executeCommand(RegisteredCommands.ADD_PLOT) + + expect(mockAddTopLevelPlot).not.to.be.called + expect(mockGetMetricAndParam).not.to.be.called + }) + + it('should not add a custom plot if user cancels during custom plot creation', async () => { const { plotsModel } = await buildPlots({ disposer: disposable, plotsDiff: plotsDiffFixture @@ -62,13 +101,15 @@ suite('Workspace Plots Test Suite', () => { customPlotQuickPickUtil, 'pickMetricAndParam' ) + const mockPickPlotType = stub(quickPickUtil, 'pickPlotType') + mockPickPlotType.onFirstCall().resolves(quickPickUtil.PLOT_TYPE.CUSTOM) mockGetMetricAndParam.onFirstCall().resolves(undefined) const mockSetCustomPlotsOrder = stub(plotsModel, 'setCustomPlotsOrder') mockSetCustomPlotsOrder.returns(undefined) - await commands.executeCommand(RegisteredCommands.PLOTS_CUSTOM_ADD) + await commands.executeCommand(RegisteredCommands.ADD_PLOT) expect(mockSetCustomPlotsOrder).to.not.be.called }) diff --git a/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx index ffb2bec82e..cc5ce425b8 100644 --- a/webview/src/plots/components/App.test.tsx +++ b/webview/src/plots/components/App.test.tsx @@ -2411,39 +2411,39 @@ describe('App', () => { ).toBeInTheDocument() }) - it('should send a message to select the revisions when clicking the filter button', () => { + it('should send a message to add a plot when clicking the add plot button', () => { renderAppWithOptionalData({ comparison: comparisonTableFixture, - sectionCollapsed: DEFAULT_SECTION_COLLAPSED + selectedRevisions: plotsRevisionsFixture }) - const filterButton = within(screen.getByTestId('ribbon')).getAllByRole( + const addPlotButton = within(screen.getByTestId('ribbon')).getAllByRole( 'button' )[0] - fireEvent.click(filterButton) + mockPostMessage.mockReset() + fireEvent.click(addPlotButton) + expect(mockPostMessage).toHaveBeenCalledTimes(1) expect(mockPostMessage).toHaveBeenCalledWith({ - type: MessageFromWebviewType.SELECT_EXPERIMENTS + type: MessageFromWebviewType.ADD_PLOT }) }) - it('should send a message to add a plot when clicking the add plot button', () => { + it('should send a message to select the revisions when clicking the filter button', () => { renderAppWithOptionalData({ comparison: comparisonTableFixture, - selectedRevisions: plotsRevisionsFixture + sectionCollapsed: DEFAULT_SECTION_COLLAPSED }) - const addPlotButton = within(screen.getByTestId('ribbon')).getAllByRole( + const filterButton = within(screen.getByTestId('ribbon')).getAllByRole( 'button' )[1] - mockPostMessage.mockReset() - fireEvent.click(addPlotButton) + fireEvent.click(filterButton) - expect(mockPostMessage).toHaveBeenCalledTimes(1) expect(mockPostMessage).toHaveBeenCalledWith({ - type: MessageFromWebviewType.ADD_PLOT + type: MessageFromWebviewType.SELECT_EXPERIMENTS }) }) diff --git a/webview/src/plots/components/ribbon/Ribbon.tsx b/webview/src/plots/components/ribbon/Ribbon.tsx index 9f9dba6f33..a174708aa2 100644 --- a/webview/src/plots/components/ribbon/Ribbon.tsx +++ b/webview/src/plots/components/ribbon/Ribbon.tsx @@ -69,19 +69,15 @@ export const Ribbon: React.FC = () => { data-testid="ribbon" className={cx(styles.list, needsShadow && styles.withShadow)} > +
  • + +
  • -
    • -
  • -
  • From 68ff206b3fc7bd057c30558a0bd2a3e320151632 Mon Sep 17 00:00:00 2001 From: julieg18 Date: Tue, 19 Sep 2023 10:26:37 -0500 Subject: [PATCH 5/8] fix typo --- extension/src/telemetry/constants.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extension/src/telemetry/constants.ts b/extension/src/telemetry/constants.ts index bc5d629026..ff5bcff127 100644 --- a/extension/src/telemetry/constants.ts +++ b/extension/src/telemetry/constants.ts @@ -86,7 +86,7 @@ export const EventName = Object.assign( VIEWS_PLOTS_COMPARISON_ROWS_REORDERED: 'views.plots.comparisonRowsReordered', VIEWS_PLOTS_CREATED: 'views.plots.created', - VIEWS_PLOTS_CUSTOM_ADD: 'views.plots.custom', + VIEWS_PLOTS_CUSTOM_ADD: 'views.plots.addCustomPlot', VIEWS_PLOTS_EXPERIMENT_TOGGLE: 'views.plots.toggleExperimentStatus', VIEWS_PLOTS_EXPORT_PLOT_DATA_AS_CSV: 'views.plots.exportPlotDataAsCsv', VIEWS_PLOTS_EXPORT_PLOT_DATA_AS_JSON: 'views.plots.exportPlotDataAsJson', From 6233c893e94893cd14c6d658a23b6a64daa04217 Mon Sep 17 00:00:00 2001 From: julieg18 Date: Tue, 19 Sep 2023 18:08:30 -0500 Subject: [PATCH 6/8] Rename "Top-Level" to "Data Series" --- extension/src/pipeline/index.ts | 2 +- extension/src/pipeline/workspace.ts | 4 ++-- extension/src/plots/quickPick.test.ts | 6 +++--- extension/src/plots/quickPick.ts | 8 ++++---- extension/src/plots/workspace.ts | 4 ++-- .../src/test/suite/pipeline/index.test.ts | 6 +++--- .../src/test/suite/plots/workspace.test.ts | 20 ++++++++++--------- 7 files changed, 26 insertions(+), 24 deletions(-) diff --git a/extension/src/pipeline/index.ts b/extension/src/pipeline/index.ts index 4ef2d5efee..d69f0944f8 100644 --- a/extension/src/pipeline/index.ts +++ b/extension/src/pipeline/index.ts @@ -112,7 +112,7 @@ export class Pipeline extends DeferredDisposable { return this.addPipeline() } - public async addTopLevelPlot() { + public async addDataSeriesPlot() { const cwd = await this.getCwd() if (!cwd) { diff --git a/extension/src/pipeline/workspace.ts b/extension/src/pipeline/workspace.ts index 6829583d68..d940075872 100644 --- a/extension/src/pipeline/workspace.ts +++ b/extension/src/pipeline/workspace.ts @@ -63,14 +63,14 @@ export class WorkspacePipeline extends BaseWorkspace { ) } - public async addTopLevelPlot(overrideRoot?: string) { + public async addDataSeriesPlot(overrideRoot?: string) { const cwd = overrideRoot || (await this.getCwd()) if (!cwd) { return } - void this.getRepository(cwd).addTopLevelPlot() + void this.getRepository(cwd).addDataSeriesPlot() } private getCwd() { diff --git a/extension/src/plots/quickPick.test.ts b/extension/src/plots/quickPick.test.ts index b3ee56c8f0..7e22b95dee 100644 --- a/extension/src/plots/quickPick.test.ts +++ b/extension/src/plots/quickPick.test.ts @@ -20,9 +20,9 @@ describe('pickPlotType', () => { [ { description: - 'Create a top-level plot definition by selecting data from a file', - label: 'Top-Level', - value: 'top-level' + 'Create a data series plot definition by selecting data from a file', + label: 'Data Series', + value: 'data-series' }, { description: diff --git a/extension/src/plots/quickPick.ts b/extension/src/plots/quickPick.ts index a37614236c..5c00508247 100644 --- a/extension/src/plots/quickPick.ts +++ b/extension/src/plots/quickPick.ts @@ -3,7 +3,7 @@ import { Title } from '../vscode/title' export const enum PLOT_TYPE { CUSTOM = 'custom', - TOP_LEVEL = 'top-level' + DATA_SERIES = 'data-series' } export const pickPlotType = () => @@ -11,9 +11,9 @@ export const pickPlotType = () => [ { description: - 'Create a top-level plot definition by selecting data from a file', - label: 'Top-Level', - value: PLOT_TYPE.TOP_LEVEL + 'Create a data series plot definition by selecting data from a file', + label: 'Data Series', + value: PLOT_TYPE.DATA_SERIES }, { description: diff --git a/extension/src/plots/workspace.ts b/extension/src/plots/workspace.ts index 008737748a..8145db9cd4 100644 --- a/extension/src/plots/workspace.ts +++ b/extension/src/plots/workspace.ts @@ -87,8 +87,8 @@ export class WorkspacePlots extends BaseWorkspaceWebviews { await this.addCustomPlot(overrideRoot) } - if (response === PLOT_TYPE.TOP_LEVEL) { - await pipelines.addTopLevelPlot(overrideRoot) + if (response === PLOT_TYPE.DATA_SERIES) { + await pipelines.addDataSeriesPlot(overrideRoot) } sendTelemetryEvent( diff --git a/extension/src/test/suite/pipeline/index.test.ts b/extension/src/test/suite/pipeline/index.test.ts index d96f6ca341..bde79ed3ff 100644 --- a/extension/src/test/suite/pipeline/index.test.ts +++ b/extension/src/test/suite/pipeline/index.test.ts @@ -289,7 +289,7 @@ suite('Pipeline Test Suite', () => { }) }) - it('should add a top-level plot', async () => { + it('should add a data series plot', async () => { const { pipeline } = buildPipeline({ disposer: disposable, dvcRoot: dvcDemoPath @@ -304,7 +304,7 @@ suite('Pipeline Test Suite', () => { mockPickPlotConfiguration.onFirstCall().resolves(undefined) - await pipeline.addTopLevelPlot() + await pipeline.addDataSeriesPlot() expect(mockPickPlotConfiguration).to.be.calledOnce expect(mockAddPlotToDvcFile).not.to.be.called @@ -316,7 +316,7 @@ suite('Pipeline Test Suite', () => { y: 'acc' }) - await pipeline.addTopLevelPlot() + await pipeline.addDataSeriesPlot() expect(mockPickPlotConfiguration).to.be.calledTwice expect(mockAddPlotToDvcFile).to.be.called diff --git a/extension/src/test/suite/plots/workspace.test.ts b/extension/src/test/suite/plots/workspace.test.ts index f1a339044c..f9ff5ab148 100644 --- a/extension/src/test/suite/plots/workspace.test.ts +++ b/extension/src/test/suite/plots/workspace.test.ts @@ -26,19 +26,21 @@ suite('Workspace Plots Test Suite', () => { }) describe('dvc.addPlot', () => { - it('should be able to add a top-level plot', async () => { + it('should be able to add a data series plot', async () => { const mockPickPlotType = stub(quickPickUtil, 'pickPlotType') - const mockAddTopLevelPlot = stub( + const mockAddDataSeriesPlot = stub( WorkspacePipeline.prototype, - 'addTopLevelPlot' + 'addDataSeriesPlot' ) - mockPickPlotType.onFirstCall().resolves(quickPickUtil.PLOT_TYPE.TOP_LEVEL) - mockAddTopLevelPlot.onFirstCall().resolves(undefined) + mockPickPlotType + .onFirstCall() + .resolves(quickPickUtil.PLOT_TYPE.DATA_SERIES) + mockAddDataSeriesPlot.onFirstCall().resolves(undefined) await commands.executeCommand(RegisteredCommands.ADD_PLOT) - expect(mockAddTopLevelPlot).to.be.calledOnce + expect(mockAddDataSeriesPlot).to.be.calledOnce }) it('should be able to add a custom plot', async () => { @@ -75,9 +77,9 @@ suite('Workspace Plots Test Suite', () => { it('should not add a plot if users cancels', async () => { const mockPickPlotType = stub(quickPickUtil, 'pickPlotType') - const mockAddTopLevelPlot = stub( + const mockAddDataSeriesPlot = stub( WorkspacePipeline.prototype, - 'addTopLevelPlot' + 'addDataSeriesPlot' ) const mockGetMetricAndParam = stub( customPlotQuickPickUtil, @@ -88,7 +90,7 @@ suite('Workspace Plots Test Suite', () => { await commands.executeCommand(RegisteredCommands.ADD_PLOT) - expect(mockAddTopLevelPlot).not.to.be.called + expect(mockAddDataSeriesPlot).not.to.be.called expect(mockGetMetricAndParam).not.to.be.called }) From ec3c5ed4fbd1c17e24c08062dd22240576f9d0b2 Mon Sep 17 00:00:00 2001 From: julieg18 Date: Wed, 20 Sep 2023 07:18:16 -0500 Subject: [PATCH 7/8] Remove + button in custom plots section --- webview/src/plots/components/App.test.tsx | 19 ------------------- .../customPlots/CustomPlotsWrapper.tsx | 15 +++------------ 2 files changed, 3 insertions(+), 31 deletions(-) diff --git a/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx index 194d9b9475..75c848cb15 100644 --- a/webview/src/plots/components/App.test.tsx +++ b/webview/src/plots/components/App.test.tsx @@ -766,25 +766,6 @@ describe('App', () => { }) }) - it('should hide the custom plots add button if there are no more plots to create', () => { - renderAppWithOptionalData({ - comparison: comparisonTableFixture, - custom: customPlotsFixture - }) - - const customSection = screen.getAllByTestId('section-container')[2] - - expect(within(customSection).getByLabelText('Add Plot')).toBeInTheDocument() - - sendSetDataMessage({ - custom: { ...customPlotsFixture, enablePlotCreation: false } - }) - - expect( - within(customSection).queryByLabelText('Add Plot') - ).not.toBeInTheDocument() - }) - it('should display a slider to pick the number of items per row if there are items and the action is available', () => { const store = renderAppWithOptionalData({ custom: customPlotsFixture diff --git a/webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx b/webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx index 3e30f79668..a8f34a0ca4 100644 --- a/webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx +++ b/webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx @@ -5,17 +5,11 @@ import { CustomPlots } from './CustomPlots' import { changeSize } from './customPlotsSlice' import { PlotsContainer } from '../PlotsContainer' import { PlotsState } from '../../store' -import { addCustomPlot, removeCustomPlots } from '../../util/messages' +import { removeCustomPlots } from '../../util/messages' export const CustomPlotsWrapper: React.FC = () => { - const { - plotsIds, - nbItemsPerRow, - isCollapsed, - height, - enablePlotCreation, - hasAddedPlots - } = useSelector((state: PlotsState) => state.custom) + const { plotsIds, nbItemsPerRow, isCollapsed, height, hasAddedPlots } = + useSelector((state: PlotsState) => state.custom) const [selectedPlots, setSelectedPlots] = useState([]) useEffect(() => { setSelectedPlots(plotsIds) @@ -29,9 +23,6 @@ export const CustomPlotsWrapper: React.FC = () => { sectionKey={PlotsSection.CUSTOM_PLOTS} nbItemsPerRowOrWidth={nbItemsPerRow} sectionCollapsed={isCollapsed} - addPlotsButton={ - enablePlotCreation ? { onClick: addCustomPlot } : undefined - } removePlotsButton={ hasAddedPlots ? { onClick: removeCustomPlots } : undefined } From 0aeb1cc951618e07395675f08ca4d813d7f2e8f9 Mon Sep 17 00:00:00 2001 From: julieg18 Date: Wed, 20 Sep 2023 07:33:44 -0500 Subject: [PATCH 8/8] Revert "Remove + button in custom plots section" This reverts commit ec3c5ed4fbd1c17e24c08062dd22240576f9d0b2. --- webview/src/plots/components/App.test.tsx | 19 +++++++++++++++++++ .../customPlots/CustomPlotsWrapper.tsx | 15 ++++++++++++--- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/webview/src/plots/components/App.test.tsx b/webview/src/plots/components/App.test.tsx index 75c848cb15..194d9b9475 100644 --- a/webview/src/plots/components/App.test.tsx +++ b/webview/src/plots/components/App.test.tsx @@ -766,6 +766,25 @@ describe('App', () => { }) }) + it('should hide the custom plots add button if there are no more plots to create', () => { + renderAppWithOptionalData({ + comparison: comparisonTableFixture, + custom: customPlotsFixture + }) + + const customSection = screen.getAllByTestId('section-container')[2] + + expect(within(customSection).getByLabelText('Add Plot')).toBeInTheDocument() + + sendSetDataMessage({ + custom: { ...customPlotsFixture, enablePlotCreation: false } + }) + + expect( + within(customSection).queryByLabelText('Add Plot') + ).not.toBeInTheDocument() + }) + it('should display a slider to pick the number of items per row if there are items and the action is available', () => { const store = renderAppWithOptionalData({ custom: customPlotsFixture diff --git a/webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx b/webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx index a8f34a0ca4..3e30f79668 100644 --- a/webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx +++ b/webview/src/plots/components/customPlots/CustomPlotsWrapper.tsx @@ -5,11 +5,17 @@ import { CustomPlots } from './CustomPlots' import { changeSize } from './customPlotsSlice' import { PlotsContainer } from '../PlotsContainer' import { PlotsState } from '../../store' -import { removeCustomPlots } from '../../util/messages' +import { addCustomPlot, removeCustomPlots } from '../../util/messages' export const CustomPlotsWrapper: React.FC = () => { - const { plotsIds, nbItemsPerRow, isCollapsed, height, hasAddedPlots } = - useSelector((state: PlotsState) => state.custom) + const { + plotsIds, + nbItemsPerRow, + isCollapsed, + height, + enablePlotCreation, + hasAddedPlots + } = useSelector((state: PlotsState) => state.custom) const [selectedPlots, setSelectedPlots] = useState([]) useEffect(() => { setSelectedPlots(plotsIds) @@ -23,6 +29,9 @@ export const CustomPlotsWrapper: React.FC = () => { sectionKey={PlotsSection.CUSTOM_PLOTS} nbItemsPerRowOrWidth={nbItemsPerRow} sectionCollapsed={isCollapsed} + addPlotsButton={ + enablePlotCreation ? { onClick: addCustomPlot } : undefined + } removePlotsButton={ hasAddedPlots ? { onClick: removeCustomPlots } : undefined }