From 40cdbe3586308df2ceafa3c93c06e779707edb13 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Tue, 23 Aug 2022 15:20:35 +1000 Subject: [PATCH 1/4] add commit and share experiment action into context menus --- extension/package.json | 17 +++- extension/package.nls.json | 1 + extension/src/cli/git/constants.ts | 2 + extension/src/cli/git/executor.test.ts | 99 +++++++++++++++++++ extension/src/cli/git/executor.ts | 26 +++-- extension/src/commands/external.ts | 1 + extension/src/experiments/commands/index.ts | 16 +++ .../src/experiments/commands/register.ts | 14 ++- extension/src/experiments/webview/messages.ts | 5 + extension/src/telemetry/constants.ts | 1 + .../src/test/suite/experiments/index.test.ts | 56 +++++++++++ extension/src/vscode/title.ts | 1 + extension/src/webview/contract.ts | 5 + .../src/experiments/components/App.test.tsx | 1 + .../src/experiments/components/table/Row.tsx | 5 + 15 files changed, 239 insertions(+), 11 deletions(-) create mode 100644 extension/src/cli/git/executor.test.ts diff --git a/extension/package.json b/extension/package.json index 07b1a83417..8e1be558c7 100644 --- a/extension/package.json +++ b/extension/package.json @@ -472,6 +472,12 @@ "category": "DVC", "icon": "$(repo-push)" }, + { + "title": "%command.views.experiments.shareExperimentAsCommit%", + "command": "dvc.views.experiments.shareExperimentAsCommit", + "category": "DVC", + "icon": "$(repo-push)" + }, { "title": "%command.views.experimentsTree.selectExperiments%", "command": "dvc.views.experimentsTree.selectExperiments", @@ -803,6 +809,10 @@ "command": "dvc.views.experiments.shareExperimentAsBranch", "when": "false" }, + { + "command": "dvc.views.experiments.shareExperimentAsCommit", + "when": "false" + }, { "command": "dvc.views.experimentsFilterByTree.removeAllFilters", "when": "false" @@ -1090,10 +1100,15 @@ "when": "view == dvc.views.experimentsTree && dvc.commands.available && viewItem =~ /^(experiment|queued)$/ && !dvc.experiment.running" }, { - "command": "dvc.views.experiments.shareExperimentAsBranch", + "command": "dvc.views.experiments.shareExperimentAsCommit", "group": "1_share@1", "when": "view == dvc.views.experimentsTree && dvc.commands.available && viewItem =~ /^(checkpoint|experiment)$/ && !dvc.experiment.running" }, + { + "command": "dvc.views.experiments.shareExperimentAsBranch", + "group": "1_share@2", + "when": "view == dvc.views.experimentsTree && dvc.commands.available && viewItem =~ /^(checkpoint|experiment)$/ && !dvc.experiment.running" + }, { "command": "dvc.views.experiments.runExperiment", "group": "2_modify@1", diff --git a/extension/package.nls.json b/extension/package.nls.json index 67fd5d9f29..c2e12b6385 100644 --- a/extension/package.nls.json +++ b/extension/package.nls.json @@ -69,6 +69,7 @@ "command.views.experiments.resumeCheckpointExperiment": "Modify Param(s) and Resume", "command.views.experiments.resetAndRunCheckpointExperiment": "Modify Param(s), Reset and Run", "command.views.experiments.shareExperimentAsBranch": "Share as Branch", + "command.views.experiments.shareExperimentAsCommit": "Commit and Share", "command.views.experimentsTree.selectExperiments": "Select Experiments to Display in Plots", "command.views.plotsPathsTree.selectPlots": "Select Plots to Display", "command.views.plotsPathsTree.refreshPlots": "Refresh Plots for Selected Experiments", diff --git a/extension/src/cli/git/constants.ts b/extension/src/cli/git/constants.ts index 312dc55e9a..61c5575ad0 100644 --- a/extension/src/cli/git/constants.ts +++ b/extension/src/cli/git/constants.ts @@ -10,6 +10,7 @@ export const HEADS_GIT_REFS = join(GIT_REFS, 'heads') export enum Command { ADD = 'add', CLEAN = 'clean', + COMMIT = 'commit', DIFF = 'diff', LS_FILES = 'ls-files', PUSH = 'push', @@ -24,6 +25,7 @@ export enum Flag { EXCLUDE_STANDARD = '--exclude-standard', FORCE = '-f', HARD = '--hard', + MESSAGE = '-m', NAME_ONLY = '--name-only', NO_EMPTY_DIRECTORY = '--no-empty-directory', OTHERS = '--others', diff --git a/extension/src/cli/git/executor.test.ts b/extension/src/cli/git/executor.test.ts new file mode 100644 index 0000000000..a911b466d9 --- /dev/null +++ b/extension/src/cli/git/executor.test.ts @@ -0,0 +1,99 @@ +import { EventEmitter } from 'vscode' +import { Disposable, Disposer } from '@hediet/std/disposable' +import { GitExecutor } from './executor' +import { createProcess } from '../../processExecution' +import { CliResult, CliStarted } from '..' +import { getMockedProcess } from '../../test/util/jest' + +jest.mock('vscode') +jest.mock('@hediet/std/disposable') +jest.mock('../../processExecution') + +const mockedDisposable = jest.mocked(Disposable) + +const mockedCreateProcess = jest.mocked(createProcess) + +beforeEach(() => { + jest.resetAllMocks() +}) + +describe('GitExecutor', () => { + mockedDisposable.fn.mockReturnValueOnce({ + track: function (disposable: T): T { + return disposable + }, + untrack: function (disposable: T): T { + return disposable + } + } as unknown as (() => void) & Disposer) + + const gitExecutor = new GitExecutor({ + processCompleted: { + event: jest.fn(), + fire: jest.fn() + } as unknown as EventEmitter, + processStarted: { + event: jest.fn(), + fire: jest.fn() + } as unknown as EventEmitter + }) + + describe('pushBranch', () => { + it('should call createProcess with the correct parameters to push a branch', async () => { + const cwd = __dirname + const branchName = 'my-branch' + mockedCreateProcess.mockReturnValueOnce( + getMockedProcess( + `branch '${branchName}' set up to track 'origin/${branchName}'.` + ) + ) + + await gitExecutor.pushBranch(cwd, branchName) + expect(mockedCreateProcess).toBeCalledWith({ + args: ['push', '--set-upstream', 'origin', branchName], + cwd, + executable: 'git' + }) + }) + + it('should call createProcess with the correct parameters to push the current branch', async () => { + const cwd = __dirname + mockedCreateProcess.mockReturnValueOnce( + getMockedProcess('Everything up-to-date') + ) + + await gitExecutor.pushBranch(cwd) + expect(mockedCreateProcess).toBeCalledWith({ + args: ['push', '--set-upstream', 'origin'], + cwd, + executable: 'git' + }) + }) + }) + + describe('stageAndCommit', () => { + it('should call createProcess with the correct parameters to stage all file and then commit', async () => { + const cwd = __dirname + const message = 'best experiment' + mockedCreateProcess.mockReturnValueOnce(getMockedProcess(cwd)) + mockedCreateProcess + .mockReturnValueOnce(getMockedProcess('')) + .mockReturnValueOnce( + getMockedProcess(`[current-branch 67effdbc] ${message}`) + ) + + await gitExecutor.stageAndCommit(cwd, message) + expect(mockedCreateProcess).toBeCalledTimes(3) + expect(mockedCreateProcess).toBeCalledWith({ + args: ['add', '.'], + cwd, + executable: 'git' + }) + expect(mockedCreateProcess).toBeCalledWith({ + args: ['commit', '-m', message], + cwd, + executable: 'git' + }) + }) + }) +}) diff --git a/extension/src/cli/git/executor.ts b/extension/src/cli/git/executor.ts index 026631d12d..74f7dc742e 100644 --- a/extension/src/cli/git/executor.ts +++ b/extension/src/cli/git/executor.ts @@ -1,5 +1,5 @@ import { GitCli } from '.' -import { Command, Commit, DEFAULT_REMOTE, Flag } from './constants' +import { Args, Command, Commit, DEFAULT_REMOTE, Flag } from './constants' import { getOptions } from './options' import { typeCheckCommands } from '..' @@ -7,6 +7,7 @@ export const autoRegisteredCommands = { GIT_PUSH_BRANCH: 'pushBranch', GIT_RESET_WORKSPACE: 'resetWorkspace', GIT_STAGE_ALL: 'stageAll', + GIT_STAGE_AND_COMMIT: 'stageAndCommit', GIT_UNSTAGE_ALL: 'reset' } as const @@ -16,6 +17,16 @@ export class GitExecutor extends GitCli { this ) + public pushBranch(cwd: string, branchName?: string) { + const args: Args = [Command.PUSH, Flag.SET_UPSTREAM, DEFAULT_REMOTE] + if (branchName) { + args.push(branchName as Commit) + } + const options = getOptions(cwd, ...args) + + return this.executeProcess(options) + } + public reset(cwd: string, ...args: (Flag | Commit)[]) { const options = getOptions(cwd, Command.RESET, ...args) @@ -43,14 +54,11 @@ export class GitExecutor extends GitCli { return this.executeProcess(options) } - public pushBranch(cwd: string, branchName: string) { - const options = getOptions( - cwd, - Command.PUSH, - Flag.SET_UPSTREAM, - DEFAULT_REMOTE, - branchName as Commit - ) + public async stageAndCommit(cwd: string, message: string) { + await this.stageAll(cwd) + + const args = [Command.COMMIT, Flag.MESSAGE, message] as Args + const options = getOptions(cwd, ...args) return this.executeProcess(options) } diff --git a/extension/src/commands/external.ts b/extension/src/commands/external.ts index 5e025138fd..e5eac9043c 100644 --- a/extension/src/commands/external.ts +++ b/extension/src/commands/external.ts @@ -16,6 +16,7 @@ export enum RegisteredCliCommands { EXPERIMENT_VIEW_BRANCH = 'dvc.views.experiments.branchExperiment', EXPERIMENT_VIEW_REMOVE = 'dvc.views.experiments.removeExperiment', EXPERIMENT_VIEW_SHARE_AS_BRANCH = 'dvc.views.experiments.shareExperimentAsBranch', + EXPERIMENT_VIEW_SHARE_AS_COMMIT = 'dvc.views.experiments.shareExperimentAsCommit', EXPERIMENT_VIEW_QUEUE = 'dvc.views.experiments.queueExperiment', EXPERIMENT_VIEW_RESUME = 'dvc.views.experiments.resumeCheckpointExperiment', diff --git a/extension/src/experiments/commands/index.ts b/extension/src/experiments/commands/index.ts index 367be050fb..3e80de376f 100644 --- a/extension/src/experiments/commands/index.ts +++ b/extension/src/experiments/commands/index.ts @@ -23,3 +23,19 @@ export const getShareExperimentAsBranchCommand = return experiments.runCommand(AvailableCommands.GIT_PUSH_BRANCH, cwd, input) } + +export const getShareExperimentAsCommitCommand = + (experiments: WorkspaceExperiments) => + async (cwd: string, name: string, input: string) => { + await experiments.runCommand(AvailableCommands.EXPERIMENT_APPLY, cwd, name) + + await experiments.runCommand( + AvailableCommands.GIT_STAGE_AND_COMMIT, + cwd, + input + ) + + await experiments.runCommand(AvailableCommands.PUSH, cwd) + + return experiments.runCommand(AvailableCommands.GIT_PUSH_BRANCH, cwd) + } diff --git a/extension/src/experiments/commands/register.ts b/extension/src/experiments/commands/register.ts index 3ae4330cfe..394f320cfb 100644 --- a/extension/src/experiments/commands/register.ts +++ b/extension/src/experiments/commands/register.ts @@ -1,6 +1,7 @@ import { getBranchExperimentCommand, - getShareExperimentAsBranchCommand + getShareExperimentAsBranchCommand, + getShareExperimentAsCommitCommand } from '.' import { pickGarbageCollectionFlags } from '../quickPick' import { WorkspaceExperiments } from '../workspace' @@ -192,6 +193,17 @@ const registerExperimentInputCommands = ( id ) ) + + internalCommands.registerExternalCliCommand( + RegisteredCliCommands.EXPERIMENT_VIEW_SHARE_AS_COMMIT, + ({ dvcRoot, id }: ExperimentDetails) => + experiments.getExpNameAndInputThenRun( + getShareExperimentAsCommitCommand(experiments), + Title.ENTER_COMMIT_MESSAGE, + dvcRoot, + id + ) + ) } const registerExperimentQuickPickCommands = ( diff --git a/extension/src/experiments/webview/messages.ts b/extension/src/experiments/webview/messages.ts index ffab126dad..e900ad4915 100644 --- a/extension/src/experiments/webview/messages.ts +++ b/extension/src/experiments/webview/messages.ts @@ -121,6 +121,11 @@ export class WebviewMessages { RegisteredCliCommands.EXPERIMENT_VIEW_SHARE_AS_BRANCH, { dvcRoot: this.dvcRoot, id: message.payload } ) + case MessageFromWebviewType.SHARE_EXPERIMENT_AS_COMMIT: + return commands.executeCommand( + RegisteredCliCommands.EXPERIMENT_VIEW_SHARE_AS_COMMIT, + { dvcRoot: this.dvcRoot, id: message.payload } + ) default: Logger.error(`Unexpected message: ${JSON.stringify(message)}`) diff --git a/extension/src/telemetry/constants.ts b/extension/src/telemetry/constants.ts index 9e898e742a..c421430b7d 100644 --- a/extension/src/telemetry/constants.ts +++ b/extension/src/telemetry/constants.ts @@ -137,6 +137,7 @@ export interface IEventNamePropertyMapping { [EventName.EXPERIMENT_VIEW_BRANCH]: undefined [EventName.EXPERIMENT_VIEW_REMOVE]: undefined [EventName.EXPERIMENT_VIEW_SHARE_AS_BRANCH]: undefined + [EventName.EXPERIMENT_VIEW_SHARE_AS_COMMIT]: undefined [EventName.EXPERIMENT_TOGGLE]: undefined [EventName.EXPERIMENT_VIEW_QUEUE]: undefined diff --git a/extension/src/test/suite/experiments/index.test.ts b/extension/src/test/suite/experiments/index.test.ts index 5f8b362249..6bf08f0706 100644 --- a/extension/src/test/suite/experiments/index.test.ts +++ b/extension/src/test/suite/experiments/index.test.ts @@ -536,6 +536,62 @@ suite('Experiments Test Suite', () => { expect(mockGitPush).to.be.calledWithExactly(dvcDemoPath, mockBranch) }).timeout(WEBVIEW_TEST_TIMEOUT) + it('should handle a message to share an experiment as a commit', async () => { + const { experiments } = buildExperiments(disposable) + await experiments.isReady() + + const testCheckpointId = 'd1343a87c6ee4a2e82d19525964d2fb2cb6756c9' + const testCheckpointLabel = shortenForLabel(testCheckpointId) + const mockCommitMessage = + 'this is the very best version that I could come up with' + const inputEvent = getInputBoxEvent(mockCommitMessage) + + const mockExperimentApply = stub( + DvcExecutor.prototype, + 'experimentApply' + ).resolves( + `Changes for experiment '${testCheckpointId}' have been applied to your current workspace.` + ) + const mockStageAndCommit = stub( + GitExecutor.prototype, + 'stageAndCommit' + ).resolves(`[current-branch 67effdbc] ${mockCommitMessage}`) + + const mockPush = stub(DvcExecutor.prototype, 'push').resolves( + '100000 files updated.' + ) + const mockGitPush = stub(GitExecutor.prototype, 'pushBranch') + const branchPushedToRemote = new Promise(resolve => + mockGitPush.callsFake(() => { + resolve(undefined) + return Promise.resolve('current-branch pushed to remote') + }) + ) + + stub(WorkspaceExperiments.prototype, 'getRepository').returns(experiments) + + const webview = await experiments.showWebview() + const mockMessageReceived = getMessageReceivedEmitter(webview) + + mockMessageReceived.fire({ + payload: testCheckpointId, + type: MessageFromWebviewType.SHARE_EXPERIMENT_AS_COMMIT + }) + + await inputEvent + await branchPushedToRemote + expect(mockStageAndCommit).to.be.calledWithExactly( + dvcDemoPath, + mockCommitMessage + ) + expect(mockExperimentApply).to.be.calledWithExactly( + dvcDemoPath, + testCheckpointLabel + ) + expect(mockPush).to.be.calledWithExactly(dvcDemoPath) + expect(mockGitPush).to.be.calledWithExactly(dvcDemoPath) + }).timeout(WEBVIEW_TEST_TIMEOUT) + it("should be able to handle a message to modify an experiment's params and queue an experiment", async () => { const { experiments, dvcExecutor } = buildExperiments(disposable) diff --git a/extension/src/vscode/title.ts b/extension/src/vscode/title.ts index ee2383411a..8e215e9bb2 100644 --- a/extension/src/vscode/title.ts +++ b/extension/src/vscode/title.ts @@ -1,6 +1,7 @@ export enum Title { CHOOSE_RESOURCES = 'Choose Resources to Add to the Dataset', ENTER_BRANCH_NAME = 'Enter a Name for the New Branch', + ENTER_COMMIT_MESSAGE = 'Enter a Commit Message', ENTER_FILTER_VALUE = 'Enter a Filter Value', ENTER_RELATIVE_DESTINATION = 'Enter a Destination Relative to the Root', GARBAGE_COLLECT_EXPERIMENTS = 'Garbage Collect Experiments', diff --git a/extension/src/webview/contract.ts b/extension/src/webview/contract.ts index 027757e064..7ccb2a07ff 100644 --- a/extension/src/webview/contract.ts +++ b/extension/src/webview/contract.ts @@ -36,6 +36,7 @@ export enum MessageFromWebviewType { SELECT_COLUMNS = 'select-columns', SELECT_PLOTS = 'select-plots', SHARE_EXPERIMENT_AS_BRANCH = 'share-experiment-as-branch', + SHARE_EXPERIMENT_AS_COMMIT = 'share-experiment-as-commit', TOGGLE_METRIC = 'toggle-metric', TOGGLE_PLOTS_SECTION = 'toggle-plots-section', MODIFY_EXPERIMENT_PARAMS_AND_QUEUE = 'modify-experiment-params-and-queue', @@ -151,6 +152,10 @@ export type MessageFromWebview = type: MessageFromWebviewType.SHARE_EXPERIMENT_AS_BRANCH payload: string } + | { + type: MessageFromWebviewType.SHARE_EXPERIMENT_AS_COMMIT + payload: string + } export type MessageToWebview = { type: MessageToWebviewType.SET_DATA diff --git a/webview/src/experiments/components/App.test.tsx b/webview/src/experiments/components/App.test.tsx index cfe9d263f1..0685afbe2d 100644 --- a/webview/src/experiments/components/App.test.tsx +++ b/webview/src/experiments/components/App.test.tsx @@ -691,6 +691,7 @@ describe('App', () => { expect(itemLabels).toStrictEqual([ 'Apply to Workspace', 'Create new Branch', + 'Commit and Share', 'Share as Branch', 'Modify, Reset and Run', 'Modify and Resume', diff --git a/webview/src/experiments/components/table/Row.tsx b/webview/src/experiments/components/table/Row.tsx index 5b3482fcf4..09774c0deb 100644 --- a/webview/src/experiments/components/table/Row.tsx +++ b/webview/src/experiments/components/table/Row.tsx @@ -181,6 +181,11 @@ const getSingleSelectMenuOptions = ( MessageFromWebviewType.CREATE_BRANCH_FROM_EXPERIMENT, isNotExperimentOrCheckpoint ), + withId( + 'Commit and Share', + MessageFromWebviewType.SHARE_EXPERIMENT_AS_COMMIT, + isNotExperimentOrCheckpoint + ), withId( 'Share as Branch', MessageFromWebviewType.SHARE_EXPERIMENT_AS_BRANCH, From eed0a53d18b87ea0c3684c35ba51b894dd693474 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Tue, 23 Aug 2022 15:33:17 +1000 Subject: [PATCH 2/4] update test statement --- extension/src/cli/git/executor.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extension/src/cli/git/executor.test.ts b/extension/src/cli/git/executor.test.ts index a911b466d9..514abd9515 100644 --- a/extension/src/cli/git/executor.test.ts +++ b/extension/src/cli/git/executor.test.ts @@ -72,7 +72,7 @@ describe('GitExecutor', () => { }) describe('stageAndCommit', () => { - it('should call createProcess with the correct parameters to stage all file and then commit', async () => { + it('should call createProcess with the correct parameters to stage all files and then commit', async () => { const cwd = __dirname const message = 'best experiment' mockedCreateProcess.mockReturnValueOnce(getMockedProcess(cwd)) From 8715e1cc8967745991dd3e8f7f6e8ac172164865 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Tue, 23 Aug 2022 15:48:32 +1000 Subject: [PATCH 3/4] use HEAD for push branch when branch name is not provided --- extension/src/cli/git/executor.test.ts | 2 +- extension/src/cli/git/executor.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/extension/src/cli/git/executor.test.ts b/extension/src/cli/git/executor.test.ts index 514abd9515..eb1e19744f 100644 --- a/extension/src/cli/git/executor.test.ts +++ b/extension/src/cli/git/executor.test.ts @@ -64,7 +64,7 @@ describe('GitExecutor', () => { await gitExecutor.pushBranch(cwd) expect(mockedCreateProcess).toBeCalledWith({ - args: ['push', '--set-upstream', 'origin'], + args: ['push', '--set-upstream', 'origin', 'HEAD'], cwd, executable: 'git' }) diff --git a/extension/src/cli/git/executor.ts b/extension/src/cli/git/executor.ts index 74f7dc742e..fe03052ab6 100644 --- a/extension/src/cli/git/executor.ts +++ b/extension/src/cli/git/executor.ts @@ -19,9 +19,9 @@ export class GitExecutor extends GitCli { public pushBranch(cwd: string, branchName?: string) { const args: Args = [Command.PUSH, Flag.SET_UPSTREAM, DEFAULT_REMOTE] - if (branchName) { - args.push(branchName as Commit) - } + + args.push((branchName || Commit.HEAD) as Commit) + const options = getOptions(cwd, ...args) return this.executeProcess(options) From 98ce49800225f25c60af1c373bebfb4994deee48 Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Wed, 24 Aug 2022 12:04:40 +1000 Subject: [PATCH 4/4] fix tests after merge main --- extension/.prettierignore | 1 + extension/src/test/suite/experiments/index.test.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/extension/.prettierignore b/extension/.prettierignore index b575855f1a..374de1622a 100644 --- a/extension/.prettierignore +++ b/extension/.prettierignore @@ -3,3 +3,4 @@ dist .wdio-vscode-service coverage src/test/fixtures/plotsDiff/vega.ts +CHANGELOG.md \ No newline at end of file diff --git a/extension/src/test/suite/experiments/index.test.ts b/extension/src/test/suite/experiments/index.test.ts index 99a288cd5a..c95fb2d9b0 100644 --- a/extension/src/test/suite/experiments/index.test.ts +++ b/extension/src/test/suite/experiments/index.test.ts @@ -563,7 +563,7 @@ suite('Experiments Test Suite', () => { }) ) - stub(WorkspaceExperiments.prototype, 'getRepository').returns(experiments) + stubWorkspaceExperimentsGetters(dvcDemoPath, experiments) const webview = await experiments.showWebview() const mockMessageReceived = getMessageReceivedEmitter(webview)