diff --git a/extension/package.json b/extension/package.json index 274f5570eb..5f4d00611e 100644 --- a/extension/package.json +++ b/extension/package.json @@ -511,6 +511,12 @@ "category": "DVC", "icon": "$(repo-push)" }, + { + "title": "Push", + "command": "dvc.views.experimentsTree.pushExperiment", + "category": "DVC", + "icon": "$(repo-push)" + }, { "title": "Show Logs", "command": "dvc.views.experiments.showLogs", @@ -863,6 +869,14 @@ "command": "dvc.views.experiments.branchExperiment", "when": "false" }, + { + "command": "dvc.views.experiments.pushExperiment", + "when": "false" + }, + { + "command": "dvc.views.experimentsTree.pushExperiment", + "when": "false" + }, { "command": "dvc.views.experiments.queueExperiment", "when": "false" @@ -887,10 +901,6 @@ "command": "dvc.views.experiments.resetAndRunCheckpointExperiment", "when": "false" }, - { - "command": "dvc.views.experiments.pushExperiment", - "when": "false" - }, { "command": "dvc.views.experiments.showLogs", "when": "false" @@ -1152,7 +1162,7 @@ "when": "view == dvc.views.experimentsTree && dvc.commands.available && viewItem =~ /^(experiment|queued)$/ && !dvc.experiment.running" }, { - "command": "dvc.views.experiments.pushExperiment", + "command": "dvc.views.experimentsTree.pushExperiment", "group": "1_share@0", "when": "view == dvc.views.experimentsTree && dvc.commands.available && viewItem == experiment && !dvc.experiment.running" }, diff --git a/extension/src/experiments/commands/index.ts b/extension/src/experiments/commands/index.ts index fac0b50f0b..2d43ae759a 100644 --- a/extension/src/experiments/commands/index.ts +++ b/extension/src/experiments/commands/index.ts @@ -44,7 +44,7 @@ const convertUrlTextToLink = (stdout: string) => { export const getPushExperimentCommand = (internalCommands: InternalCommands, setup: Setup) => - ({ dvcRoot, id }: { dvcRoot: string; id: string }) => { + ({ dvcRoot, ids }: { dvcRoot: string; ids: string[] }) => { const studioAccessToken = setup.getStudioAccessToken() if ( !( @@ -58,14 +58,14 @@ export const getPushExperimentCommand = return Toast.showProgress('exp push', async progress => { progress.report({ increment: 0 }) - progress.report({ increment: 25, message: `Pushing ${id}...` }) + progress.report({ increment: 25, message: `Pushing ${ids.join(' ')}...` }) const remainingProgress = 75 const stdout = await internalCommands.executeCommand( AvailableCommands.EXP_PUSH, dvcRoot, - id + ...ids ) progress.report({ diff --git a/extension/src/experiments/model/collect.ts b/extension/src/experiments/model/collect.ts index 35101876e8..94985d1186 100644 --- a/extension/src/experiments/model/collect.ts +++ b/extension/src/experiments/model/collect.ts @@ -328,10 +328,10 @@ export const collectExperiments = ( return acc } -type DeletableExperimentAccumulator = { [dvcRoot: string]: Set } +type ExperimentTypesAccumulator = { [dvcRoot: string]: Set } const initializeAccumulatorRoot = ( - acc: DeletableExperimentAccumulator, + acc: ExperimentTypesAccumulator, dvcRoot: string ) => { if (!acc[dvcRoot]) { @@ -340,12 +340,12 @@ const initializeAccumulatorRoot = ( } const collectExperimentItem = ( - acc: DeletableExperimentAccumulator, - deletable: Set, + acc: ExperimentTypesAccumulator, + types: Set, experimentItem: ExperimentItem ) => { const { dvcRoot, type, id, label } = experimentItem - if (!deletable.has(type)) { + if (!types.has(type)) { return } initializeAccumulatorRoot(acc, dvcRoot) @@ -357,18 +357,17 @@ const collectExperimentItem = ( acc[dvcRoot].add(id) } -export const collectDeletable = ( - experimentItems: (string | ExperimentItem)[] -): DeletableExperimentAccumulator => { - const deletable = new Set([ExperimentType.EXPERIMENT, ExperimentType.QUEUED]) - - const acc: DeletableExperimentAccumulator = {} +export const collectExperimentType = ( + experimentItems: (string | ExperimentItem)[], + types: Set +): ExperimentTypesAccumulator => { + const acc: ExperimentTypesAccumulator = {} for (const experimentItem of experimentItems) { if (typeof experimentItem === 'string') { continue } - collectExperimentItem(acc, deletable, experimentItem) + collectExperimentItem(acc, types, experimentItem) } return acc diff --git a/extension/src/experiments/model/tree.ts b/extension/src/experiments/model/tree.ts index 9f63199817..0b6693468f 100644 --- a/extension/src/experiments/model/tree.ts +++ b/extension/src/experiments/model/tree.ts @@ -9,7 +9,7 @@ import { Uri } from 'vscode' import { ExperimentType } from '.' -import { collectDeletable, ExperimentItem } from './collect' +import { collectExperimentType, ExperimentItem } from './collect' import { MAX_SELECTED_EXPERIMENTS } from './status' import { getDataFromColumnPaths } from './util' import { WorkspaceExperiments } from '../workspace' @@ -69,7 +69,7 @@ export class ExperimentsTree this.experiments = experiments this.resourceLocator = resourceLocator - this.registerWorkaroundCommand() + this.registerWorkaroundCommands() this.updateDescriptionOnChange() } @@ -123,24 +123,44 @@ export class ExperimentsTree ) } - private registerWorkaroundCommand() { + private registerWorkaroundCommands() { + const callCommandWithSelected = async ( + command: + | RegisteredCliCommands.EXPERIMENT_VIEW_REMOVE + | RegisteredCliCommands.EXPERIMENT_VIEW_PUSH, + experimentItem: ExperimentItem | string, + types: ExperimentType[] + ) => { + const selected = [ + ...this.getSelectedExperimentItems(), + experimentItem + ] as (string | ExperimentItem)[] + + const acc = collectExperimentType(selected, new Set(types)) + + for (const [dvcRoot, ids] of Object.entries(acc)) { + await commands.executeCommand(command, { dvcRoot, ids: [...ids] }) + } + } + commands.registerCommand( 'dvc.views.experimentsTree.removeExperiment', - async experimentItem => { - const selected = [ - ...this.getSelectedExperimentItems(), - experimentItem - ] as (string | ExperimentItem)[] - - const deletable = collectDeletable(selected) - - for (const [dvcRoot, ids] of Object.entries(deletable)) { - await commands.executeCommand( - RegisteredCliCommands.EXPERIMENT_VIEW_REMOVE, - { dvcRoot, ids } - ) - } - } + (experimentItem: ExperimentItem) => + callCommandWithSelected( + RegisteredCliCommands.EXPERIMENT_VIEW_REMOVE, + experimentItem, + [ExperimentType.EXPERIMENT, ExperimentType.QUEUED] + ) + ) + + commands.registerCommand( + 'dvc.views.experimentsTree.pushExperiment', + (experimentItem: ExperimentItem) => + callCommandWithSelected( + RegisteredCliCommands.EXPERIMENT_VIEW_PUSH, + experimentItem, + [ExperimentType.EXPERIMENT] + ) ) } diff --git a/extension/src/experiments/webview/messages.ts b/extension/src/experiments/webview/messages.ts index a43297c480..202e8ce289 100644 --- a/extension/src/experiments/webview/messages.ts +++ b/extension/src/experiments/webview/messages.ts @@ -197,7 +197,7 @@ export class WebviewMessages { case MessageFromWebviewType.PUSH_EXPERIMENT: return commands.executeCommand( RegisteredCliCommands.EXPERIMENT_VIEW_PUSH, - { dvcRoot: this.dvcRoot, id: message.payload } + { dvcRoot: this.dvcRoot, ids: message.payload } ) case MessageFromWebviewType.SHOW_EXPERIMENT_LOGS: diff --git a/extension/src/test/suite/experiments/index.test.ts b/extension/src/test/suite/experiments/index.test.ts index 8efd84fe9d..daff6d1ac2 100644 --- a/extension/src/test/suite/experiments/index.test.ts +++ b/extension/src/test/suite/experiments/index.test.ts @@ -671,7 +671,7 @@ suite('Experiments Test Suite', () => { ) mockMessageReceived.fire({ - payload: mockExpId, + payload: [mockExpId], type: MessageFromWebviewType.PUSH_EXPERIMENT }) @@ -712,7 +712,7 @@ suite('Experiments Test Suite', () => { ) mockMessageReceived.fire({ - payload: mockExpId, + payload: [mockExpId], type: MessageFromWebviewType.PUSH_EXPERIMENT }) diff --git a/extension/src/test/suite/experiments/model/tree.test.ts b/extension/src/test/suite/experiments/model/tree.test.ts index c30ae742aa..9b3ad3764b 100644 --- a/extension/src/test/suite/experiments/model/tree.test.ts +++ b/extension/src/test/suite/experiments/model/tree.test.ts @@ -12,6 +12,7 @@ import { ExperimentType } from '../../../../experiments/model' import { UNSELECTED } from '../../../../experiments/model/status' import { bypassProcessManagerDebounce, + bypassProgressCloseDelay, getFirstArgOfLastCall, getMockNow, getTimeSafeDisposer, @@ -345,6 +346,99 @@ suite('Experiments Tree Test Suite', () => { ) }) + it('should be able to push an experiment with dvc.views.experimentsTree.pushExperiment', async () => { + bypassProgressCloseDelay() + const mockExperimentId = 'exp-to-push' + const mockExperiment = { + dvcRoot: dvcDemoPath, + id: mockExperimentId, + type: ExperimentType.EXPERIMENT + } + + const mockExpPush = stub(DvcExecutor.prototype, 'expPush').resolves('') + + stubPrivatePrototypeMethod( + ExperimentsTree, + 'getSelectedExperimentItems' + ).returns([mockExperiment]) + + await commands.executeCommand( + 'dvc.views.experimentsTree.pushExperiment', + mockExperiment + ) + + expect(mockExpPush).to.be.calledWithExactly(dvcDemoPath, mockExperimentId) + }) + + it('should be able to push the provided experiment with dvc.views.experimentsTree.pushExperiment (if no experiments are selected)', async () => { + bypassProgressCloseDelay() + const mockExperiment = 'exp-to-push' + + const mockExpPush = stub(DvcExecutor.prototype, 'expPush').resolves('') + + stubPrivatePrototypeMethod( + ExperimentsTree, + 'getSelectedExperimentItems' + ).returns([]) + + await commands.executeCommand( + 'dvc.views.experimentsTree.pushExperiment', + { + dvcRoot: dvcDemoPath, + id: mockExperiment, + type: ExperimentType.EXPERIMENT + } + ) + + expect(mockExpPush).to.be.calledWithExactly(dvcDemoPath, mockExperiment) + }) + + it('should be able to push multiple experiments with dvc.views.experimentsTree.pushExperiment', async () => { + bypassProgressCloseDelay() + const mockFirstExperimentId = 'first-exp-pushed' + const mockSecondExperimentId = 'second-exp-pushed' + const mockQueuedExperimentLabel = 'queued-excluded' + + const mockExpPush = stub(DvcExecutor.prototype, 'expPush').resolves('') + + stubPrivatePrototypeMethod( + ExperimentsTree, + 'getSelectedExperimentItems' + ).returns([ + dvcDemoPath, + { + dvcRoot: dvcDemoPath, + label: mockQueuedExperimentLabel, + type: ExperimentType.QUEUED + }, + { + dvcRoot: dvcDemoPath, + id: mockFirstExperimentId, + type: ExperimentType.EXPERIMENT + }, + { + dvcRoot: dvcDemoPath, + id: 'workspace-excluded', + type: ExperimentType.WORKSPACE + } + ]) + + await commands.executeCommand( + 'dvc.views.experimentsTree.pushExperiment', + { + dvcRoot: dvcDemoPath, + id: mockSecondExperimentId, + type: ExperimentType.EXPERIMENT + } + ) + + expect(mockExpPush).to.be.calledWithExactly( + dvcDemoPath, + mockFirstExperimentId, + mockSecondExperimentId + ) + }) + it('should be able to apply an experiment to the workspace with dvc.views.experiments.applyExperiment', async () => { const { experiments } = buildExperiments(disposable) diff --git a/extension/src/webview/contract.ts b/extension/src/webview/contract.ts index 4ae7329feb..3c712cf445 100644 --- a/extension/src/webview/contract.ts +++ b/extension/src/webview/contract.ts @@ -155,7 +155,7 @@ export type MessageFromWebview = | { type: MessageFromWebviewType.SHOW_EXPERIMENT_LOGS; payload: string } | { type: MessageFromWebviewType.PUSH_EXPERIMENT - payload: string + payload: string[] } | { type: MessageFromWebviewType.REMOVE_COLUMN_SORT diff --git a/webview/src/experiments/components/App.test.tsx b/webview/src/experiments/components/App.test.tsx index 7cb43dc8f1..cd2f6d14c2 100644 --- a/webview/src/experiments/components/App.test.tsx +++ b/webview/src/experiments/components/App.test.tsx @@ -801,6 +801,7 @@ describe('App', () => { }) }) + // eslint-disable-next-line sonarjs/cognitive-complexity describe('Row Context Menu', () => { beforeAll(() => { jest.useFakeTimers() @@ -932,7 +933,7 @@ describe('App', () => { expect(itemLabels).toContain('Remove') }) - it('should present the Remove option if multiple checkpoint tip rows are selected', () => { + it('should present the remove option if only experiments are selected', () => { renderTableWithoutRunningExperiments() clickRowCheckbox('4fb124a') @@ -944,10 +945,10 @@ describe('App', () => { advanceTimersByTime(100) const menuitems = screen.getAllByRole('menuitem') const itemLabels = menuitems.map(item => item.textContent) - expect(itemLabels).toContain('Remove Selected Rows') + expect(itemLabels).toContain('Remove Selected') const removeOption = menuitems.find(item => - item.textContent?.includes('Remove Selected Rows') + item.textContent?.includes('Remove Selected') ) expect(removeOption).toBeDefined() @@ -960,6 +961,34 @@ describe('App', () => { }) }) + it('should present the push option if only experiments are selected', () => { + renderTableWithoutRunningExperiments() + + clickRowCheckbox('4fb124a') + clickRowCheckbox('42b8736') + + const target = screen.getByText('4fb124a') + fireEvent.contextMenu(target, { bubbles: true }) + + advanceTimersByTime(100) + const menuitems = screen.getAllByRole('menuitem') + const itemLabels = menuitems.map(item => item.textContent) + expect(itemLabels).toContain('Push Selected') + + const pushOption = menuitems.find(item => + item.textContent?.includes('Push Selected') + ) + + expect(pushOption).toBeDefined() + + pushOption && fireEvent.click(pushOption) + + expect(sendMessage).toHaveBeenCalledWith({ + payload: ['exp-e7a67', 'test-branch'], + type: MessageFromWebviewType.PUSH_EXPERIMENT + }) + }) + it('should present the stop option if only running experiments are selected', () => { renderTable() @@ -1034,7 +1063,7 @@ describe('App', () => { shareOption && fireEvent.click(shareOption) expect(sendMessage).toHaveBeenCalledWith({ - payload: 'exp-e7a67', + payload: ['exp-e7a67'], type: MessageFromWebviewType.PUSH_EXPERIMENT }) }) @@ -1106,8 +1135,13 @@ describe('App', () => { fireEvent.contextMenu(target, { bubbles: true }) advanceTimersByTime(100) - const clearOption = screen.getByText('Clear row selection') - fireEvent.click(clearOption) + + const menuitems = screen.getAllByRole('menuitem') + + const clearOption = menuitems.find(item => + item.textContent?.includes('Clear') + ) + clearOption && fireEvent.click(clearOption) advanceTimersByTime(100) expect(selectedRows().length).toBe(0) diff --git a/webview/src/experiments/components/table/body/RowContextMenu.tsx b/webview/src/experiments/components/table/body/RowContextMenu.tsx index 0d81ac6947..f2000e5fcd 100644 --- a/webview/src/experiments/components/table/body/RowContextMenu.tsx +++ b/webview/src/experiments/components/table/body/RowContextMenu.tsx @@ -50,12 +50,12 @@ const getMultiSelectMenuOptions = ( const selectedIds = selectedRowsList.map(value => value.row.original.id) - const removableRowIds = selectedRowsList + const experimentRowIds = selectedRowsList .filter(value => value.row.depth === 1) .map(value => value.row.original.id) - const hideRemoveOption = - removableRowIds.length !== selectedRowsList.length || hasRunningExperiment + const hideExperimentOnlyOption = + experimentRowIds.length !== selectedRowsList.length || hasRunningExperiment const stoppableRows = selectedRowsList .filter(value => isRunning(value.row.original.status)) @@ -105,17 +105,24 @@ const getMultiSelectMenuOptions = ( true ), experimentMenuOption( - removableRowIds, - 'Remove Selected Rows', + experimentRowIds, + 'Push Selected', + MessageFromWebviewType.PUSH_EXPERIMENT, + hideExperimentOnlyOption, + true + ), + experimentMenuOption( + experimentRowIds, + 'Remove Selected', MessageFromWebviewType.REMOVE_EXPERIMENT, - hideRemoveOption, + hideExperimentOnlyOption, true ), { divider: true, id: 'clear-selection', keyboardShortcut: 'Esc', - label: 'Clear row selection' + label: 'Clear' } ] } @@ -200,9 +207,11 @@ const getSingleSelectMenuOptions = ( 'Create new Branch', MessageFromWebviewType.CREATE_BRANCH_FROM_EXPERIMENT ), - hideIfRunningOrNotExperiment( + experimentMenuOption( + [id], 'Push', MessageFromWebviewType.PUSH_EXPERIMENT, + isNotExperiment, true ), ...getRunResumeOptions(