diff --git a/extension/src/cli/git/constants.ts b/extension/src/cli/git/constants.ts index 65d6e5578e..b630fc0c51 100644 --- a/extension/src/cli/git/constants.ts +++ b/extension/src/cli/git/constants.ts @@ -11,6 +11,7 @@ export const gitPath = { export enum Command { ADD = 'add', + BRANCH = 'branch', CLEAN = 'clean', COMMIT = 'commit', DIFF = 'diff', @@ -37,6 +38,7 @@ export enum Flag { MESSAGE = '-m', NAME_ONLY = '--name-only', NO_EMPTY_DIRECTORY = '--no-empty-directory', + NO_MERGE = '--no-merge', NUMBER = '-n', PRETTY_FORMAT_COMMIT_MESSAGE = '--pretty=format:%H%n%an%n%ar%nrefNames:%D%nmessage:%B', OTHERS = '--others', diff --git a/extension/src/cli/git/reader.test.ts b/extension/src/cli/git/reader.test.ts new file mode 100644 index 0000000000..3bd3f18639 --- /dev/null +++ b/extension/src/cli/git/reader.test.ts @@ -0,0 +1,71 @@ +import { EventEmitter } from 'vscode' +import { Disposable, Disposer } from '@hediet/std/disposable' +import { GitReader } from './reader' +import { CliResult, CliStarted } from '..' +import { createProcess } from '../../process/execution' +import { getMockedProcess } from '../../test/util/jest' + +jest.mock('vscode') +jest.mock('@hediet/std/disposable') +jest.mock('fs') +jest.mock('../../process/execution') +jest.mock('../../env') +jest.mock('../../common/logger') + +const mockedDisposable = jest.mocked(Disposable) + +const mockedCreateProcess = jest.mocked(createProcess) + +beforeEach(() => { + jest.resetAllMocks() +}) + +describe('GitReader', () => { + mockedDisposable.fn.mockReturnValueOnce({ + track: function (disposable: T): T { + return disposable + }, + untrack: function (disposable: T): T { + return disposable + } + } as unknown as (() => void) & Disposer) + + const gitReader = new GitReader({ + processCompleted: { + event: jest.fn(), + fire: jest.fn() + } as unknown as EventEmitter, + processStarted: { + event: jest.fn(), + fire: jest.fn() + } as unknown as EventEmitter + }) + + describe('getBranches', () => { + it('should match the expected output', async () => { + const cwd = __dirname + const branches = ['main', 'exp-12', 'fix-bug-11', 'other'] + mockedCreateProcess.mockReturnValueOnce( + getMockedProcess(branches.join('\n')) + ) + + const cliOutput = await gitReader.getBranches(cwd) + expect(cliOutput).toStrictEqual(branches) + expect(mockedCreateProcess).toHaveBeenCalledWith({ + args: ['branch', '--no-merge'], + cwd, + executable: 'git' + }) + }) + + it('should return an empty array if the cli returns any type of error', async () => { + const cwd = __dirname + mockedCreateProcess.mockImplementationOnce(() => { + throw new Error('unexpected error - something something') + }) + + const cliOutput = await gitReader.getBranches(cwd) + expect(cliOutput).toStrictEqual([]) + }) + }) +}) diff --git a/extension/src/cli/git/reader.ts b/extension/src/cli/git/reader.ts index 120e5dac7b..68e2136527 100644 --- a/extension/src/cli/git/reader.ts +++ b/extension/src/cli/git/reader.ts @@ -7,6 +7,7 @@ import { trimAndSplit } from '../../util/stdout' import { isDirectory } from '../../fileSystem' export const autoRegisteredCommands = { + GIT_GET_BRANCHES: 'getBranches', GIT_GET_COMMIT_MESSAGES: 'getCommitMessages', GIT_GET_NUM_COMMITS: 'getNumCommits', GIT_GET_REMOTE_URL: 'getRemoteUrl', @@ -88,12 +89,22 @@ export class GitReader extends GitCli { ) try { const revisions = await this.executeProcess(options) - return revisions.split('\n').length + return trimAndSplit(revisions).length } catch { return '' } } + public async getBranches(cwd: string): Promise { + const options = getOptions(cwd, Command.BRANCH, Flag.NO_MERGE) + try { + const branches = await this.executeProcess(options) + return trimAndSplit(branches) + } catch { + return [] + } + } + private async getUntrackedDirectories(cwd: string): Promise { const options = getOptions( cwd, diff --git a/extension/src/experiments/data/index.ts b/extension/src/experiments/data/index.ts index 24e0c3f099..14269c1efd 100644 --- a/extension/src/experiments/data/index.ts +++ b/extension/src/experiments/data/index.ts @@ -45,6 +45,7 @@ export class ExperimentsData extends BaseData { void this.watchExpGitRefs() void this.managedUpdate(QUEUED_EXPERIMENT_PATH) + void this.updateAvailableBranchesToSelect() } public managedUpdate(path?: string) { @@ -81,6 +82,14 @@ export class ExperimentsData extends BaseData { this.collectedFiles = collectFiles(data, this.collectedFiles) } + private async updateAvailableBranchesToSelect() { + const allBranches = await this.internalCommands.executeCommand( + AvailableCommands.GIT_GET_BRANCHES, + this.dvcRoot + ) + this.experiments.setAvailableBranchesToShow(allBranches) + } + private async watchExpGitRefs(): Promise { const gitRoot = await this.internalCommands.executeCommand( AvailableCommands.GIT_GET_REPOSITORY_ROOT, @@ -106,6 +115,7 @@ export class ExperimentsData extends BaseData { if ( watchedRelPaths.some(watchedRelPath => path.includes(watchedRelPath)) ) { + void this.updateAvailableBranchesToSelect() return this.managedUpdate(path) } } diff --git a/extension/src/experiments/index.ts b/extension/src/experiments/index.ts index 076c8e3f36..ff8ba5e727 100644 --- a/extension/src/experiments/index.ts +++ b/extension/src/experiments/index.ts @@ -106,6 +106,9 @@ export class Experiments extends BaseRepository { private dvcLiveOnlySignalFile: string private readonly addStage: () => Promise + private readonly selectBranches: ( + branchesSelected: string[] + ) => Promise constructor( dvcRoot: string, @@ -114,6 +117,9 @@ export class Experiments extends BaseRepository { resourceLocator: ResourceLocator, workspaceState: Memento, addStage: () => Promise, + selectBranches: ( + branchesSelected: string[] + ) => Promise, cliData?: ExperimentsData, fileSystemData?: FileSystemData ) { @@ -126,6 +132,7 @@ export class Experiments extends BaseRepository { this.internalCommands = internalCommands this.addStage = addStage + this.selectBranches = selectBranches this.onDidChangeIsParamsFileFocused = this.paramsFileFocused.event this.onDidChangeExperiments = this.experimentsChanged.event @@ -579,6 +586,7 @@ export class Experiments extends BaseRepository { this.dvcRoot ), () => this.addStage(), + (branchesSelected: string[]) => this.selectBranches(branchesSelected), () => this.internalCommands.executeCommand( AvailableCommands.GIT_GET_NUM_COMMITS, diff --git a/extension/src/experiments/model/index.ts b/extension/src/experiments/model/index.ts index 719083486f..1f80d2e5ef 100644 --- a/extension/src/experiments/model/index.ts +++ b/extension/src/experiments/model/index.ts @@ -57,6 +57,8 @@ export class ExperimentsModel extends ModelWithPersistence { private starredExperiments: StarredExperiments private numberOfCommitsToShow: number private isBranchesView: boolean + private branchesToShow: string[] = [] + private availableBranchesToShow: string[] = [] private filters: Map = new Map() @@ -92,6 +94,11 @@ export class ExperimentsModel extends ModelWithPersistence { DEFAULT_NUM_OF_COMMITS_TO_SHOW ) + this.branchesToShow = this.revive( + PersistenceKey.EXPERIMENTS_BRANCHES, + [] + ) + const assignedColors = new Set( Object.values(this.coloredStatus).filter(Boolean) ) @@ -403,6 +410,23 @@ export class ExperimentsModel extends ModelWithPersistence { return this.isBranchesView } + public setBranchesToShow(branches: string[]) { + this.branchesToShow = branches + this.persistBranchesToShow() + } + + public getBranchesToShow() { + return this.branchesToShow + } + + public setAvailableBranchesToShow(branches: string[]) { + this.availableBranchesToShow = branches + } + + public getAvailableBranchesToShow() { + return this.availableBranchesToShow + } + private findIndexByPath(pathToRemove: string) { return this.currentSorts.findIndex(({ path }) => path === pathToRemove) } @@ -507,6 +531,13 @@ export class ExperimentsModel extends ModelWithPersistence { ) } + private persistBranchesToShow() { + return this.persist( + PersistenceKey.EXPERIMENTS_BRANCHES, + this.branchesToShow + ) + } + private addDetails(experiment: Experiment) { const { id } = experiment diff --git a/extension/src/experiments/webview/contract.ts b/extension/src/experiments/webview/contract.ts index cec1eab8a4..e48fb8c5fe 100644 --- a/extension/src/experiments/webview/contract.ts +++ b/extension/src/experiments/webview/contract.ts @@ -97,6 +97,7 @@ export type TableData = { columnWidths: Record filteredCount: number filters: string[] + hasBranchesToSelect: boolean hasCheckpoints: boolean hasColumns: boolean hasConfig: boolean diff --git a/extension/src/experiments/webview/messages.ts b/extension/src/experiments/webview/messages.ts index 754eefdedd..442c9aaa05 100644 --- a/extension/src/experiments/webview/messages.ts +++ b/extension/src/experiments/webview/messages.ts @@ -50,6 +50,10 @@ export class WebviewMessages { private isShowingMoreCommits = true private readonly addStage: () => Promise + private readonly selectBranches: ( + branchesSelected: string[] + ) => Promise + private readonly getNumCommits: () => Promise private readonly update: () => Promise @@ -67,6 +71,9 @@ export class WebviewMessages { ) => Promise, hasStages: () => Promise, addStage: () => Promise, + selectBranches: ( + branchesSelected: string[] + ) => Promise, getNumCommits: () => Promise, update: () => Promise ) { @@ -80,6 +87,7 @@ export class WebviewMessages { this.stopQueuedExperiments = stopQueuedExperiments this.hasStages = hasStages this.addStage = addStage + this.selectBranches = selectBranches this.getNumCommits = getNumCommits this.update = update @@ -221,6 +229,8 @@ export class WebviewMessages { case MessageFromWebviewType.SHOW_LESS_COMMITS: return this.changeCommitsToShow(-1) + case MessageFromWebviewType.SELECT_BRANCHES: + return this.addAndRemoveBranches() case MessageFromWebviewType.SWITCH_BRANCHES_VIEW: return this.switchToBranchesView() @@ -233,6 +243,17 @@ export class WebviewMessages { } } + private async addAndRemoveBranches() { + const selectedBranches = await this.selectBranches( + this.experiments.getBranchesToShow() + ) + if (!selectedBranches) { + return + } + this.experiments.setBranchesToShow(selectedBranches) + await this.update() + } + private async switchToBranchesView() { this.experiments.setIsBranchesView(true) await this.update() @@ -269,6 +290,8 @@ export class WebviewMessages { columns: this.columns.getSelected(), filteredCount: this.experiments.getFilteredCount(), filters: this.experiments.getFilterPaths(), + hasBranchesToSelect: + this.experiments.getAvailableBranchesToShow().length > 0, hasCheckpoints: this.checkpoints.hasCheckpoints(), hasColumns: this.columns.hasNonDefaultColumns(), hasConfig: this.hasConfig, diff --git a/extension/src/experiments/workspace.test.ts b/extension/src/experiments/workspace.test.ts index d2cdf18d38..91f8a35207 100644 --- a/extension/src/experiments/workspace.test.ts +++ b/extension/src/experiments/workspace.test.ts @@ -1,7 +1,11 @@ import { Disposable, Disposer } from '@hediet/std/disposable' import { Experiments } from '.' import { scriptCommand, WorkspaceExperiments } from './workspace' -import { quickPickOne, quickPickOneOrInput } from '../vscode/quickPick' +import { + quickPickManyValues, + quickPickOne, + quickPickOneOrInput +} from '../vscode/quickPick' import { CommandId, AvailableCommands, @@ -26,6 +30,7 @@ const mockedDisposable = jest.mocked(Disposable) const mockedDvcRoot = '/my/dvc/root' const mockedOtherDvcRoot = '/my/fun/dvc/root' const mockedQuickPickOne = jest.mocked(quickPickOne) +const mockedQuickPickManyValues = jest.mocked(quickPickManyValues) const mockedQuickPickOneOrInput = jest.mocked(quickPickOneOrInput) const mockedGetValidInput = jest.mocked(getValidInput) const mockedPickExperiment = jest.fn() @@ -36,6 +41,7 @@ const mockedListStages = jest.fn() const mockedFindOrCreateDvcYamlFile = jest.mocked(findOrCreateDvcYamlFile) const mockedGetFileExtension = jest.mocked(getFileExtension) const mockedHasDvcYamlFile = jest.mocked(hasDvcYamlFile) +const mockedGetBranches = jest.fn() const mockedPickFile = jest.mocked(pickFile) jest.mock('vscode') @@ -74,6 +80,11 @@ describe('Experiments', () => { mockedListStages() ) + mockedInternalCommands.registerCommand( + AvailableCommands.GIT_GET_BRANCHES, + () => mockedGetBranches() + ) + const mockedUpdatesPaused = buildMockedEventEmitter() const workspaceExperiments = new WorkspaceExperiments( @@ -714,4 +725,121 @@ describe('Experiments', () => { expect(showErrorSpy).not.toHaveBeenCalled() }) }) + + describe('selectBranches', () => { + it('should get all the branches from GIT_GET_BRANCHES command', async () => { + mockedQuickPickOne.mockResolvedValueOnce(mockedDvcRoot) + mockedGetBranches.mockResolvedValueOnce(['main']) + + await workspaceExperiments.selectBranches([]) + + expect(mockedGetBranches).toHaveBeenCalledTimes(1) + }) + + it('should show a quick pick to select many values when called', async () => { + mockedQuickPickOne.mockResolvedValueOnce(mockedDvcRoot) + mockedGetBranches.mockResolvedValueOnce(['main']) + + await workspaceExperiments.selectBranches([]) + + expect(mockedQuickPickManyValues).toHaveBeenCalledTimes(1) + }) + + it('should display all branches in the quick pick', async () => { + const allBranches = [ + 'main', + 'special-branch', + 'important-fix', + 'exp-best' + ] + mockedQuickPickOne.mockResolvedValueOnce(mockedDvcRoot) + mockedGetBranches.mockResolvedValueOnce(allBranches) + + await workspaceExperiments.selectBranches([]) + + expect(mockedQuickPickManyValues).toHaveBeenCalledWith( + allBranches.map(branch => + expect.objectContaining({ label: branch, value: branch }) + ), + expect.anything() + ) + }) + + it('should not display the current branch in the quick pick', async () => { + const allBranches = [ + '* WIP', + 'main', + 'special-branch', + 'important-fix', + 'exp-best' + ] + mockedQuickPickOne.mockResolvedValue(mockedDvcRoot) + mockedGetBranches.mockResolvedValueOnce(allBranches) + + await workspaceExperiments.selectBranches([]) + + expect(mockedQuickPickManyValues).toHaveBeenCalledWith( + allBranches + .slice(1) + .map(branch => + expect.objectContaining({ label: branch, value: branch }) + ), + expect.anything() + ) + + mockedQuickPickManyValues.mockResolvedValueOnce([]) + + const updatedAllBranches = [ + 'main', + '* special-branch', + 'important-fix', + 'exp-best' + ] + + mockedGetBranches.mockResolvedValueOnce(updatedAllBranches) + + await workspaceExperiments.selectBranches([]) + + expect(mockedQuickPickManyValues).toHaveBeenCalledWith( + [...updatedAllBranches.slice(0, 1), ...updatedAllBranches.slice(2)].map( + branch => expect.objectContaining({ label: branch, value: branch }) + ), + expect.anything() + ) + }) + + it('should mark the selected branches as picked in the quick pick', async () => { + const allBranches = [ + 'main', + 'special-branch', + 'important-fix', + 'exp-best' + ] + const selectedBranches = ['main', 'exp-best'] + mockedQuickPickOne.mockResolvedValue(mockedDvcRoot) + mockedGetBranches.mockResolvedValueOnce(allBranches) + + await workspaceExperiments.selectBranches(selectedBranches) + + expect(mockedQuickPickManyValues).toHaveBeenCalledWith( + [ + { label: 'main', picked: true, value: 'main' }, + { label: 'special-branch', picked: false, value: 'special-branch' }, + { label: 'important-fix', picked: false, value: 'important-fix' }, + { label: 'exp-best', picked: true, value: 'exp-best' } + ], + expect.anything() + ) + }) + + it('should return early if no dvcRoot is selected', async () => { + mockedQuickPickOne.mockResolvedValue(undefined) + mockedGetBranches.mockResolvedValueOnce([]) + + await workspaceExperiments.selectBranches([]) + + expect(mockedGetBranches).not.toHaveBeenCalled() + expect(mockedQuickPickManyValues).not.toHaveBeenCalled() + }) + }) }) diff --git a/extension/src/experiments/workspace.ts b/extension/src/experiments/workspace.ts index a2e2dc928a..1c35289a85 100644 --- a/extension/src/experiments/workspace.ts +++ b/extension/src/experiments/workspace.ts @@ -23,7 +23,7 @@ import { getFileExtension, hasDvcYamlFile } from '../fileSystem' -import { quickPickOneOrInput } from '../vscode/quickPick' +import { quickPickManyValues, quickPickOneOrInput } from '../vscode/quickPick' import { pickFile } from '../vscode/resourcePicker' export enum scriptCommand { @@ -355,7 +355,8 @@ export class WorkspaceExperiments extends BaseWorkspaceWebviews< updatesPaused, resourceLocator, this.workspaceState, - () => this.checkOrAddPipeline(dvcRoot) + () => this.checkOrAddPipeline(dvcRoot), + (branchesSelected: string[]) => this.selectBranches(branchesSelected) ) ) @@ -430,6 +431,29 @@ export class WorkspaceExperiments extends BaseWorkspaceWebviews< ) } + public async selectBranches(branchesSelected: string[]) { + const cwd = await this.getDvcRoot() + if (!cwd) { + return + } + const allBranches = await this.internalCommands.executeCommand( + AvailableCommands.GIT_GET_BRANCHES, + cwd + ) + return await quickPickManyValues( + allBranches + .filter(branch => branch.indexOf('*') !== 0) + .map(branch => ({ + label: branch, + picked: branchesSelected.includes(branch), + value: branch + })), + { + title: Title.SELECT_BRANCHES + } + ) + } + private async shouldRun() { const cwd = await this.getFocusedOrOnlyOrPickProject() if (!cwd) { diff --git a/extension/src/fileSystem/watcher.ts b/extension/src/fileSystem/watcher.ts index aeb5362f69..a12a3b7115 100644 --- a/extension/src/fileSystem/watcher.ts +++ b/extension/src/fileSystem/watcher.ts @@ -20,6 +20,7 @@ export const createFileSystemWatcher = ( 'FileSystemWatcher will not behave as expected under these circumstances.' ) } + const fileSystemWatcher = workspace.createFileSystemWatcher(glob) track(fileSystemWatcher) track(fileSystemWatcher.onDidCreate(uri => listener(uri.fsPath))) diff --git a/extension/src/persistence/constants.ts b/extension/src/persistence/constants.ts index 960d68bb11..f92dd1c668 100644 --- a/extension/src/persistence/constants.ts +++ b/extension/src/persistence/constants.ts @@ -1,4 +1,5 @@ export enum PersistenceKey { + EXPERIMENTS_BRANCHES = 'experimentsBranches:', EXPERIMENTS_FILTER_BY = 'experimentsFilterBy:', EXPERIMENTS_SORT_BY = 'experimentsSortBy:', EXPERIMENTS_STATUS = 'experimentsStatus:', diff --git a/extension/src/test/fixtures/expShow/base/tableData.ts b/extension/src/test/fixtures/expShow/base/tableData.ts index 11acadcd00..39fc2f8a52 100644 --- a/extension/src/test/fixtures/expShow/base/tableData.ts +++ b/extension/src/test/fixtures/expShow/base/tableData.ts @@ -9,6 +9,7 @@ const tableDataFixture: TableData = { columnWidths: {}, filteredCount: 0, filters: [], + hasBranchesToSelect: true, hasCheckpoints: true, hasColumns: true, hasConfig: true, diff --git a/extension/src/test/fixtures/expShow/dataTypes/tableData.ts b/extension/src/test/fixtures/expShow/dataTypes/tableData.ts index f939113f82..96f519ea62 100644 --- a/extension/src/test/fixtures/expShow/dataTypes/tableData.ts +++ b/extension/src/test/fixtures/expShow/dataTypes/tableData.ts @@ -9,6 +9,7 @@ export const data: TableData = { columnWidths: {}, filteredCount: 0, filters: [], + hasBranchesToSelect: true, hasCheckpoints: false, hasColumns: true, hasConfig: true, diff --git a/extension/src/test/fixtures/expShow/deeplyNested/tableData.ts b/extension/src/test/fixtures/expShow/deeplyNested/tableData.ts index b61990c50c..695c14915b 100644 --- a/extension/src/test/fixtures/expShow/deeplyNested/tableData.ts +++ b/extension/src/test/fixtures/expShow/deeplyNested/tableData.ts @@ -12,6 +12,7 @@ const data: TableData = { 'params:params.yaml:nested1.doubled', 'params:params.yaml:nested1%2Enested2%2Enested3.nested4.nested5b.doubled' ], + hasBranchesToSelect: true, hasCheckpoints: false, hasColumns: true, hasConfig: true, diff --git a/extension/src/test/fixtures/expShow/survival/tableData.ts b/extension/src/test/fixtures/expShow/survival/tableData.ts index b1a56bcde9..5447b20897 100644 --- a/extension/src/test/fixtures/expShow/survival/tableData.ts +++ b/extension/src/test/fixtures/expShow/survival/tableData.ts @@ -9,6 +9,7 @@ const data: TableData = { columnWidths: {}, filteredCount: 0, filters: [], + hasBranchesToSelect: true, hasCheckpoints: true, hasColumns: true, hasConfig: true, diff --git a/extension/src/test/suite/experiments/index.test.ts b/extension/src/test/suite/experiments/index.test.ts index cb832f73ac..b8f7d697e6 100644 --- a/extension/src/test/suite/experiments/index.test.ts +++ b/extension/src/test/suite/experiments/index.test.ts @@ -324,16 +324,27 @@ suite('Experiments Test Suite', () => { dvcExecutor, mockCheckOrAddPipeline, messageSpy, - mockUpdateExperimentsData + mockUpdateExperimentsData, + mockSelectBranches } = buildExperiments(disposable, expShowFixture) const mockExecuteCommand = stub( internalCommands, 'executeCommand' - ).callsFake(commandId => - commandId === AvailableCommands.GIT_GET_COMMIT_MESSAGES - ? Promise.resolve('') - : Promise.resolve(undefined) - ) + ).callsFake(commandId => { + switch (commandId) { + case AvailableCommands.GIT_GET_COMMIT_MESSAGES: + return Promise.resolve('') + case AvailableCommands.GIT_GET_BRANCHES: + return Promise.resolve([ + 'main', + 'other', + 'one-more', + 'important-fix' + ]) + default: + return Promise.resolve(undefined) + } + }) return { columnsModel, dvcExecutor, @@ -342,6 +353,7 @@ suite('Experiments Test Suite', () => { messageSpy, mockCheckOrAddPipeline, mockExecuteCommand, + mockSelectBranches, mockUpdateExperimentsData } } @@ -1422,6 +1434,63 @@ suite('Experiments Test Suite', () => { expect(mockUpdateExperimentsData).to.be.calledOnce expect(experimentsModel.getIsBranchesView()).to.be.false }).timeout(WEBVIEW_TEST_TIMEOUT) + + it('should handle a message to select branches', async () => { + const { + experiments, + experimentsModel, + messageSpy, + mockUpdateExperimentsData, + mockSelectBranches + } = setupExperimentsAndMockCommands() + const mockSetBranchesToShow = stub(experimentsModel, 'setBranchesToShow') + + const waitForBranchesToBeSelected = new Promise(resolve => + mockSetBranchesToShow.callsFake(() => resolve(undefined)) + ) + + const webview = await experiments.showWebview() + messageSpy.resetHistory() + const mockMessageReceived = getMessageReceivedEmitter(webview) + + mockMessageReceived.fire({ + type: MessageFromWebviewType.SELECT_BRANCHES + }) + + expect(mockSelectBranches).to.be.calledOnce + + await waitForBranchesToBeSelected + + expect(mockSetBranchesToShow).to.be.calledOnceWith(['main', 'other']) + + expect(mockUpdateExperimentsData).to.be.calledOnce + }).timeout(WEBVIEW_TEST_TIMEOUT) + + it('should not update the selected branches when the user closes the select branches quick pick', async () => { + const { + experiments, + experimentsModel, + messageSpy, + mockUpdateExperimentsData, + mockSelectBranches + } = setupExperimentsAndMockCommands() + const mockSetBranchesToShow = stub(experimentsModel, 'setBranchesToShow') + mockSelectBranches.resolves(undefined) + + const webview = await experiments.showWebview() + messageSpy.resetHistory() + const mockMessageReceived = getMessageReceivedEmitter(webview) + + mockMessageReceived.fire({ + type: MessageFromWebviewType.SELECT_BRANCHES + }) + + expect(mockSelectBranches).to.be.calledOnce + + expect(mockSetBranchesToShow).not.to.be.calledOnceWith(['main', 'other']) + + expect(mockUpdateExperimentsData).not.to.be.calledOnce + }).timeout(WEBVIEW_TEST_TIMEOUT) }) describe('Sorting', () => { @@ -1451,6 +1520,7 @@ suite('Experiments Test Suite', () => { resourceLocator, buildMockMemento(), () => Promise.resolve(true), + () => Promise.resolve([]), buildMockData(), buildMockData() ) @@ -1668,6 +1738,7 @@ suite('Experiments Test Suite', () => { {} as ResourceLocator, mockMemento, () => Promise.resolve(true), + () => Promise.resolve([]), buildMockData(), buildMockData() ) @@ -1826,6 +1897,7 @@ suite('Experiments Test Suite', () => { {} as ResourceLocator, mockMemento, () => Promise.resolve(true), + () => Promise.resolve([]), buildMockData(), buildMockData() ) diff --git a/extension/src/test/suite/experiments/util.ts b/extension/src/test/suite/experiments/util.ts index 6fa31d206c..29138bcb1a 100644 --- a/extension/src/test/suite/experiments/util.ts +++ b/extension/src/test/suite/experiments/util.ts @@ -68,7 +68,7 @@ export const buildExperiments = ( mockUpdateExperimentsData ) const mockCheckOrAddPipeline = stub() - + const mockSelectBranches = stub().resolves(['main', 'other']) const experiments = disposer.track( new Experiments( dvcRoot, @@ -77,6 +77,7 @@ export const buildExperiments = ( resourceLocator, buildMockMemento(), mockCheckOrAddPipeline, + mockSelectBranches, mockExperimentsData, buildMockData() ) @@ -104,6 +105,7 @@ export const buildExperiments = ( mockExperimentShow, mockExperimentsData, mockGetCommitMessages, + mockSelectBranches, mockUpdateExperimentsData, resourceLocator, updatesPaused diff --git a/extension/src/test/suite/plots/util.ts b/extension/src/test/suite/plots/util.ts index ec6714418f..a2cf510ffe 100644 --- a/extension/src/test/suite/plots/util.ts +++ b/extension/src/test/suite/plots/util.ts @@ -49,6 +49,7 @@ export const buildPlots = async ( resourceLocator, buildMockMemento(), () => Promise.resolve(true), + () => Promise.resolve([]), buildMockData(), buildMockData() ) diff --git a/extension/src/vscode/title.ts b/extension/src/vscode/title.ts index eecb9ff650..b06a41427f 100644 --- a/extension/src/vscode/title.ts +++ b/extension/src/vscode/title.ts @@ -12,6 +12,7 @@ export enum Title { ENTER_STAGE_NAME = 'Enter a name for the main stage of your pipeline', GARBAGE_COLLECT_EXPERIMENTS = 'Garbage Collect Experiments', SHOW_SETUP = 'Show Setup', + SELECT_BRANCHES = 'Select the Branch(es) to Display in the Experiments Table', SELECT_BASE_EXPERIMENT = 'Select an Experiment to Use as a Base', SELECT_COLUMNS = 'Select Columns to Display in the Experiments Table', SELECT_EXPERIMENT = 'Select an Experiment', diff --git a/extension/src/webview/contract.ts b/extension/src/webview/contract.ts index 59f8d6afc1..c7947fd7e7 100644 --- a/extension/src/webview/contract.ts +++ b/extension/src/webview/contract.ts @@ -69,7 +69,8 @@ export enum MessageFromWebviewType { SHOW_MORE_COMMITS = 'show-more-commits', SHOW_LESS_COMMITS = 'show-less-commits', SWITCH_BRANCHES_VIEW = 'show-all-branches', - SWITCH_COMMITS_VIEW = 'show-commits' + SWITCH_COMMITS_VIEW = 'show-commits', + SELECT_BRANCHES = 'select-branches' } export type ColumnResizePayload = { @@ -236,6 +237,7 @@ export type MessageFromWebview = | { type: MessageFromWebviewType.SHOW_LESS_COMMITS } | { type: MessageFromWebviewType.SWITCH_BRANCHES_VIEW } | { type: MessageFromWebviewType.SWITCH_COMMITS_VIEW } + | { type: MessageFromWebviewType.SELECT_BRANCHES } 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 add9df1f96..e2d713bc39 100644 --- a/webview/src/experiments/components/App.test.tsx +++ b/webview/src/experiments/components/App.test.tsx @@ -54,12 +54,14 @@ import { import { clearSelection, createWindowTextSelection } from '../../test/selection' import { sendMessage } from '../../shared/vscode' import { setExperimentsAsStarred } from '../../test/tableDataFixture' +import { featureFlag } from '../../util/flags' jest.mock('../../shared/api') jest.mock('../../util/styles') jest.mock('./overflowHoverTooltip/useIsFullyContained', () => ({ useIsFullyContained: jest.fn() })) +const mockedFeatureFlags = jest.mocked(featureFlag) const mockedUseIsFullyContained = jest.mocked(useIsFullyContained) const { postMessage } = vsCodeApi @@ -1549,21 +1551,23 @@ describe('App', () => { it('should show a button to switch to branches view if isBranchesView is set to false', () => { renderTable({ ...tableDataFixture, isBranchesView: false }) - expect(screen.getByText('Switch to Branches View')).toBeInTheDocument() + expect( + screen.getByText('Switch to All Branches View') + ).toBeInTheDocument() }) it('should not show a button to switch to branches view if isBranchesView is set to true', () => { renderTable({ ...tableDataFixture, isBranchesView: true }) expect( - screen.queryByText('Switch to Branches View') + screen.queryByText('Switch to All Branches View') ).not.toBeInTheDocument() }) it('should send a message to switch to branches view when clicking the switch to branches view button', () => { renderTable({ ...tableDataFixture, isBranchesView: false }) - fireEvent.click(screen.getByText('Switch to Branches View')) + fireEvent.click(screen.getByText('Switch to All Branches View')) expect(mockPostMessage).toHaveBeenCalledWith({ type: MessageFromWebviewType.SWITCH_BRANCHES_VIEW @@ -1620,4 +1624,50 @@ describe('App', () => { ) }) }) + + describe('Add / Remove branches', () => { + beforeEach(() => { + mockedFeatureFlags.ADD_REMOVE_BRANCHES = true + }) + + it('should send a message to select branches when clicking the select branches button', () => { + renderTable() + + fireEvent.click(screen.getByText('Select Branch(es) to Show')) + + expect(mockPostMessage).toHaveBeenCalledWith({ + type: MessageFromWebviewType.SELECT_BRANCHES + }) + }) + + it('should disable the select branches button if there are no branches to select', () => { + renderTable({ ...tableDataFixture, hasBranchesToSelect: false }) + + fireEvent.click(screen.getByText('Select Branch(es) to Show')) + + expect(mockPostMessage).not.toHaveBeenCalledWith({ + type: MessageFromWebviewType.SELECT_BRANCHES + }) + }) + }) + + describe('Feature flags', () => { + it('should not show the add/remove branches buttons if the feature flag is off', () => { + mockedFeatureFlags.ADD_REMOVE_BRANCHES = false + + renderTable() + + expect( + screen.queryByText('Select Branch(es) to Show') + ).not.toBeInTheDocument() + }) + + it('should show the add/remove branches buttons if the feature flag is on', () => { + mockedFeatureFlags.ADD_REMOVE_BRANCHES = true + + renderTable() + + expect(screen.getByText('Select Branch(es) to Show')).toBeInTheDocument() + }) + }) }) diff --git a/webview/src/experiments/components/App.tsx b/webview/src/experiments/components/App.tsx index a1495cb2df..ed16ad3e15 100644 --- a/webview/src/experiments/components/App.tsx +++ b/webview/src/experiments/components/App.tsx @@ -14,6 +14,7 @@ import { updateColumnWidths, updateFilteredCount, updateFilters, + updateHasBranchesToSelect, updateHasCheckpoints, updateHasColumns, updateHasConfig, @@ -56,6 +57,11 @@ export const App: React.FC> = () => { case 'filters': dispatch(updateFilters(data.data.filters)) continue + case 'hasBranchesToSelect': + dispatch( + updateHasBranchesToSelect(data.data.hasBranchesToSelect) + ) + continue case 'hasCheckpoints': dispatch(updateHasCheckpoints(data.data.hasCheckpoints)) continue diff --git a/webview/src/experiments/components/table/Table.tsx b/webview/src/experiments/components/table/Table.tsx index 00c83c8e1c..9af3ad4f7d 100644 --- a/webview/src/experiments/components/table/Table.tsx +++ b/webview/src/experiments/components/table/Table.tsx @@ -14,7 +14,7 @@ import { InstanceProp, RowProp } from './interfaces' import { RowSelectionContext } from './RowSelectionContext' import { TableBody } from './TableBody' import { Indicators } from './Indicators' -import { CommitsAndBranchesNavigation } from './CommitsAndBranchesNavigation' +import { CommitsAndBranchesNavigation } from './commitsAndBranches/CommitsAndBranchesNavigation' import { ExperimentsState } from '../../store' interface TableProps extends InstanceProp { diff --git a/webview/src/experiments/components/table/commitsAndBranches/AddAndRemoveBranches.tsx b/webview/src/experiments/components/table/commitsAndBranches/AddAndRemoveBranches.tsx new file mode 100644 index 0000000000..e3edc91a84 --- /dev/null +++ b/webview/src/experiments/components/table/commitsAndBranches/AddAndRemoveBranches.tsx @@ -0,0 +1,26 @@ +import React from 'react' +import { useSelector } from 'react-redux' +import styles from '../styles.module.scss' +import { ExperimentsState } from '../../../store' +import { selectBranches } from '../messages' +import { featureFlag } from '../../../../util/flags' + +export const AddAndRemoveBranches: React.FC = () => { + const { hasBranchesToSelect } = useSelector( + (state: ExperimentsState) => state.tableData + ) + + if (!featureFlag.ADD_REMOVE_BRANCHES) { + return null + } + + return ( + + ) +} diff --git a/webview/src/experiments/components/table/CommitsAndBranchesNavigation.tsx b/webview/src/experiments/components/table/commitsAndBranches/CommitsAndBranchesNavigation.tsx similarity index 78% rename from webview/src/experiments/components/table/CommitsAndBranchesNavigation.tsx rename to webview/src/experiments/components/table/commitsAndBranches/CommitsAndBranchesNavigation.tsx index 4cef119b74..a3dccbfa21 100644 --- a/webview/src/experiments/components/table/CommitsAndBranchesNavigation.tsx +++ b/webview/src/experiments/components/table/commitsAndBranches/CommitsAndBranchesNavigation.tsx @@ -1,13 +1,14 @@ import React from 'react' import { useSelector } from 'react-redux' +import { AddAndRemoveBranches } from './AddAndRemoveBranches' import { showLessCommits, showMoreCommits, switchToBranchesView, switchToCommitsView -} from './messages' -import styles from './styles.module.scss' -import { ExperimentsState } from '../../store' +} from '../messages' +import styles from '../styles.module.scss' +import { ExperimentsState } from '../../../store' export const CommitsAndBranchesNavigation: React.FC = () => { const { hasMoreCommits, isBranchesView, isShowingMoreCommits } = useSelector( @@ -34,13 +35,18 @@ export const CommitsAndBranchesNavigation: React.FC = () => { Show Less Commits )} + + + ) diff --git a/webview/src/experiments/components/table/messages.ts b/webview/src/experiments/components/table/messages.ts index 7faa713b2c..06253b36f1 100644 --- a/webview/src/experiments/components/table/messages.ts +++ b/webview/src/experiments/components/table/messages.ts @@ -32,3 +32,8 @@ export const switchToBranchesView = () => { export const switchToCommitsView = () => { sendMessage({ type: MessageFromWebviewType.SWITCH_COMMITS_VIEW }) } + +export const selectBranches = () => + sendMessage({ + type: MessageFromWebviewType.SELECT_BRANCHES + }) diff --git a/webview/src/experiments/components/table/tableDataSlice.ts b/webview/src/experiments/components/table/tableDataSlice.ts index 1e38249c1d..4f84017db5 100644 --- a/webview/src/experiments/components/table/tableDataSlice.ts +++ b/webview/src/experiments/components/table/tableDataSlice.ts @@ -19,6 +19,7 @@ export const tableDataInitialState: TableDataState = { columns: [], filteredCount: 0, filters: [], + hasBranchesToSelect: true, hasCheckpoints: false, hasColumns: false, hasConfig: false, @@ -70,6 +71,9 @@ export const tableDataSlice = createSlice({ updateFilters: (state, action: PayloadAction) => { state.filters = action.payload }, + updateHasBranchesToSelect: (state, action: PayloadAction) => { + state.hasBranchesToSelect = action.payload + }, updateHasCheckpoints: (state, action: PayloadAction) => { state.hasCheckpoints = action.payload }, @@ -120,6 +124,7 @@ export const { updateColumns, updateFilteredCount, updateFilters, + updateHasBranchesToSelect, updateHasCheckpoints, updateHasColumns, updateHasConfig, diff --git a/webview/src/stories/Table.stories.tsx b/webview/src/stories/Table.stories.tsx index b9a990d839..87d2f69010 100644 --- a/webview/src/stories/Table.stories.tsx +++ b/webview/src/stories/Table.stories.tsx @@ -42,6 +42,7 @@ const tableData: TableDataState = { columns: columnsFixture, filteredCount: 0, filters: ['params:params.yaml:lr'], + hasBranchesToSelect: true, hasCheckpoints: true, hasColumns: true, hasConfig: true, diff --git a/webview/src/test/sort.ts b/webview/src/test/sort.ts index 1818627e83..2f64d34b61 100644 --- a/webview/src/test/sort.ts +++ b/webview/src/test/sort.ts @@ -46,6 +46,7 @@ export const tableData: TableData = { columns: columns as Column[], filteredCount: 0, filters: [], + hasBranchesToSelect: true, hasCheckpoints: false, hasColumns: true, hasConfig: true, diff --git a/webview/src/util/flags.ts b/webview/src/util/flags.ts new file mode 100644 index 0000000000..1199ecad07 --- /dev/null +++ b/webview/src/util/flags.ts @@ -0,0 +1,3 @@ +export const featureFlag = { + ADD_REMOVE_BRANCHES: true +}