diff --git a/extension/src/cli/dvc/reader.ts b/extension/src/cli/dvc/reader.ts index 4ef291076c..ed6b7b5472 100644 --- a/extension/src/cli/dvc/reader.ts +++ b/extension/src/cli/dvc/reader.ts @@ -127,7 +127,7 @@ export class DvcReader extends DvcCli { } catch (error: unknown) { const msg = (error as MaybeConsoleError).stderr || (error as Error).message - Logger.error(`${args} failed with ${msg} retrying...`) + Logger.error(`${args} failed with ${msg}`) return { error: { msg, type: 'Caught error' } } } } diff --git a/extension/src/plots/data/index.ts b/extension/src/plots/data/index.ts index 1b02d38722..8af2eb5ad5 100644 --- a/extension/src/plots/data/index.ts +++ b/extension/src/plots/data/index.ts @@ -2,6 +2,7 @@ import { EventEmitter } from 'vscode' import { PlotsOutput } from '../../cli/dvc/contract' import { AvailableCommands, InternalCommands } from '../../commands/internal' import { BaseData } from '../../data' +import { Experiments } from '../../experiments' import { definedAndNonEmpty, flattenUnique, @@ -10,12 +11,14 @@ import { import { PlotsModel } from '../model' export class PlotsData extends BaseData<{ data: PlotsOutput; revs: string[] }> { - private readonly model: PlotsModel + private readonly plots: PlotsModel + private readonly experiments: Experiments constructor( dvcRoot: string, internalCommands: InternalCommands, - model: PlotsModel, + plots: PlotsModel, + experiments: Experiments, updatesPaused: EventEmitter ) { super( @@ -30,13 +33,15 @@ export class PlotsData extends BaseData<{ data: PlotsOutput; revs: string[] }> { ], ['dvc.yaml', 'dvc.lock'] ) - this.model = model + + this.plots = plots + this.experiments = experiments } public async update(): Promise { const revs = flattenUnique([ - this.model.getMissingRevisions(), - this.model.getMutableRevisions() + this.plots.getMissingRevisions(this.experiments.getSelectedRevisions()), + this.experiments.getMutableRevisions() ]) if ( @@ -49,17 +54,13 @@ export class PlotsData extends BaseData<{ data: PlotsOutput; revs: string[] }> { } const args = this.getArgs(revs) - const data = await this.internalCommands.executeCommand( - AvailableCommands.PLOTS_DIFF, - this.dvcRoot, - ...args - ) + const data = await this.fetch(args) const files = this.collectFiles({ data }) this.compareFiles(files) - return this.notifyChanged({ data, revs }) + this.notifyChanged({ data, revs }) } public managedUpdate() { @@ -70,9 +71,17 @@ export class PlotsData extends BaseData<{ data: PlotsOutput; revs: string[] }> { return Object.keys(data) } + public fetch(revs: string[]) { + return this.internalCommands.executeCommand( + AvailableCommands.PLOTS_DIFF, + this.dvcRoot, + ...revs + ) + } + private getArgs(revs: string[]) { const cliWillThrowError = sameContents(revs, ['workspace']) - if (this.model && cliWillThrowError) { + if (this.plots && cliWillThrowError) { return [] } diff --git a/extension/src/plots/index.ts b/extension/src/plots/index.ts index 0400ad7a6b..1c5488f971 100644 --- a/extension/src/plots/index.ts +++ b/extension/src/plots/index.ts @@ -12,12 +12,13 @@ import { BaseRepository } from '../webview/repository' import { Experiments } from '../experiments' import { Resource } from '../resourceLocator' import { InternalCommands } from '../commands/internal' -import { definedAndNonEmpty } from '../util/array' +import { definedAndNonEmpty, sameContents } from '../util/array' import { ExperimentsOutput } from '../cli/dvc/contract' import { TEMP_PLOTS_DIR } from '../cli/dvc/constants' import { removeDir } from '../fileSystem' import { Toast } from '../vscode/toast' import { pickPaths } from '../path/selection/quickPick' +import { SelectedExperimentWithColor } from '../experiments/model' export type PlotsWebview = BaseWebview @@ -48,7 +49,7 @@ export class Plots extends BaseRepository { this.experiments = experiments this.plots = this.dispose.track( - new PlotsModel(this.dvcRoot, experiments, workspaceState) + new PlotsModel(this.dvcRoot, workspaceState) ) this.paths = this.dispose.track( new PathsModel(this.dvcRoot, workspaceState) @@ -61,7 +62,13 @@ export class Plots extends BaseRepository { ) this.data = this.dispose.track( - new PlotsData(dvcRoot, internalCommands, this.plots, updatesPaused) + new PlotsData( + dvcRoot, + internalCommands, + this.plots, + this.experiments, + updatesPaused + ) ) this.onDidUpdateData() @@ -77,7 +84,7 @@ export class Plots extends BaseRepository { } public sendInitialWebviewData() { - return this.fetchMissingOrSendPlots() + return this.fetchMissingAndSendPlots() } public togglePathStatus(path: string) { @@ -104,7 +111,9 @@ export class Plots extends BaseRepository { Toast.infoWithOptions( 'Attempting to refresh plots for selected experiments.' ) - for (const { revision } of this.plots.getSelectedRevisionDetails()) { + for (const { revision } of this.plots.getSelectedRevisionDetails( + this.experiments.getSelectedRevisions() + )) { this.plots.setupManualRefresh(revision) } this.data.managedUpdate() @@ -130,21 +139,42 @@ export class Plots extends BaseRepository { private notifyChanged() { this.pathsChanged.fire() - this.fetchMissingOrSendPlots() + this.fetchMissingAndSendPlots() } - private async fetchMissingOrSendPlots() { + private async fetchMissingAndSendPlots( + overrideRevs?: SelectedExperimentWithColor[] + ) { await this.isReady() + const selectedRevs = overrideRevs || this.experiments.getSelectedRevisions() + const selectedExperiments = this.experiments.getSelectedExperiments() + + const missingRevs = this.plots.getMissingRevisions(selectedRevs) + + if (this.paths.hasPaths() && definedAndNonEmpty(missingRevs)) { + const data = await this.data.fetch(missingRevs) + + await Promise.all([ + this.plots.transformAndSetPlots(data, missingRevs, selectedRevs), + this.paths.transformAndSet(data) + ]) + } + + this.webviewMessages.sendWebviewMessage(selectedRevs, selectedExperiments) + return this.syncWebview(selectedRevs) + } + + private syncWebview(selectedRevs: SelectedExperimentWithColor[]) { if ( - this.paths.hasPaths() && - definedAndNonEmpty(this.plots.getUnfetchedRevisions()) + sameContents( + this.experiments.getSelectedRevisions().map(({ label }) => label), + selectedRevs.map(({ label }) => label) + ) ) { - this.webviewMessages.sendCheckpointPlotsMessage() - return this.data.managedUpdate() + return } - - return this.webviewMessages.sendWebviewMessage() + this.fetchMissingAndSendPlots() } private createWebviewMessageHandler( @@ -184,19 +214,30 @@ export class Plots extends BaseRepository { private setupExperimentsListener(experiments: Experiments) { this.dispose.track( experiments.onDidChangeExperiments(async data => { + const selectedRevs = experiments.getSelectedRevisions() if (data) { - await this.plots.transformAndSetExperiments(data) + await this.plots.transformAndSetExperiments( + data, + experiments.getBranchRevisions(), + experiments.getRevisions(), + experiments.hasCheckpoints() + ) } - this.plots.setComparisonOrder() + this.plots.setComparisonOrder(selectedRevs) - this.fetchMissingOrSendPlots() + this.fetchMissingAndSendPlots(selectedRevs) }) ) } private async initializeData(data: ExperimentsOutput) { - await this.plots.transformAndSetExperiments(data) + await this.plots.transformAndSetExperiments( + data, + this.experiments.getBranchRevisions(), + this.experiments.getRevisions(), + this.experiments.hasCheckpoints() + ) this.data.managedUpdate() await Promise.all([ this.data.isReady(), @@ -210,7 +251,11 @@ export class Plots extends BaseRepository { this.dispose.track( this.data.onDidUpdate(async ({ data, revs }) => { await Promise.all([ - this.plots.transformAndSetPlots(data, revs), + this.plots.transformAndSetPlots( + data, + revs, + this.experiments.getSelectedRevisions() + ), this.paths.transformAndSet(data) ]) this.notifyChanged() diff --git a/extension/src/plots/model/index.test.ts b/extension/src/plots/model/index.test.ts index c0aca7f906..c560fd75bb 100644 --- a/extension/src/plots/model/index.test.ts +++ b/extension/src/plots/model/index.test.ts @@ -6,8 +6,8 @@ import { Section } from '../webview/contract' import { buildMockMemento } from '../../test/util' -import { Experiments } from '../../experiments' import { PersistenceKey } from '../../persistence/constants' +import { SelectedExperimentWithColor } from '../../experiments/model' const mockedRevisions = [ { displayColor: 'white', label: 'workspace' }, @@ -15,7 +15,7 @@ const mockedRevisions = [ { displayColor: 'blue', label: '71f31cf' }, { displayColor: 'black', label: 'e93c7e6' }, { displayColor: 'brown', label: 'ffbe811' } -] +] as unknown as SelectedExperimentWithColor[] describe('plotsModel', () => { let model: PlotsModel @@ -26,17 +26,9 @@ describe('plotsModel', () => { persistedSelectedMetrics, [PersistenceKey.PLOT_SIZES + exampleDvcRoot]: DEFAULT_SECTION_SIZES }) - const mockedGetSelectedRevisions = jest.fn() beforeEach(() => { - model = new PlotsModel( - exampleDvcRoot, - { - getSelectedRevisions: mockedGetSelectedRevisions, - isReady: () => Promise.resolve(undefined) - } as unknown as Experiments, - memento - ) + model = new PlotsModel(exampleDvcRoot, memento) jest.clearAllMocks() }) @@ -112,11 +104,9 @@ describe('plotsModel', () => { }) it('should reorder comparison revisions after receiving a message to reorder', () => { - mockedGetSelectedRevisions.mockReturnValue(mockedRevisions) - const mementoUpdateSpy = jest.spyOn(memento, 'update') const newOrder = ['71f31cf', 'e93c7e6', 'ffbe811', 'workspace', 'main'] - model.setComparisonOrder(newOrder) + model.setComparisonOrder(mockedRevisions, newOrder) expect(mementoUpdateSpy).toHaveBeenCalledTimes(1) expect(mementoUpdateSpy).toHaveBeenCalledWith( @@ -125,19 +115,21 @@ describe('plotsModel', () => { ) expect( - model.getSelectedRevisionDetails().map(({ revision }) => revision) + model + .getSelectedRevisionDetails(mockedRevisions) + .map(({ revision }) => revision) ).toStrictEqual(newOrder) }) it('should always send new revisions to the end of the list', () => { - mockedGetSelectedRevisions.mockReturnValue(mockedRevisions) - const newOrder = ['71f31cf', 'e93c7e6'] - model.setComparisonOrder(newOrder) + model.setComparisonOrder(mockedRevisions, newOrder) expect( - model.getSelectedRevisionDetails().map(({ revision }) => revision) + model + .getSelectedRevisionDetails(mockedRevisions) + .map(({ revision }) => revision) ).toStrictEqual([ ...newOrder, ...mockedRevisions @@ -151,31 +143,29 @@ describe('plotsModel', () => { const revisionDropped = allRevisions.filter(({ label }) => label !== 'main') const revisionReAdded = allRevisions - mockedGetSelectedRevisions - .mockReturnValueOnce(allRevisions) - .mockReturnValueOnce(allRevisions) - .mockReturnValueOnce(revisionDropped) - .mockReturnValueOnce(revisionDropped) - .mockReturnValueOnce(revisionReAdded) - .mockReturnValueOnce(revisionReAdded) - const initialOrder = ['workspace', 'main', '71f31cf'] - model.setComparisonOrder(initialOrder) + model.setComparisonOrder(allRevisions, initialOrder) expect( - model.getSelectedRevisionDetails().map(({ revision }) => revision) + model + .getSelectedRevisionDetails(allRevisions) + .map(({ revision }) => revision) ).toStrictEqual(initialOrder) - model.setComparisonOrder() + model.setComparisonOrder(revisionDropped) expect( - model.getSelectedRevisionDetails().map(({ revision }) => revision) + model + .getSelectedRevisionDetails(revisionDropped) + .map(({ revision }) => revision) ).toStrictEqual(initialOrder.filter(revision => revision !== 'main')) - model.setComparisonOrder() + model.setComparisonOrder(revisionReAdded) expect( - model.getSelectedRevisionDetails().map(({ revision }) => revision) + model + .getSelectedRevisionDetails(revisionReAdded) + .map(({ revision }) => revision) ).toStrictEqual(['workspace', '71f31cf', 'main']) }) }) diff --git a/extension/src/plots/model/index.ts b/extension/src/plots/model/index.ts index d0f44cc154..d374c8412e 100644 --- a/extension/src/plots/model/index.ts +++ b/extension/src/plots/model/index.ts @@ -26,7 +26,6 @@ import { PlotSizeNumber } from '../webview/contract' import { ExperimentsOutput, PlotsOutput } from '../../cli/dvc/contract' -import { Experiments } from '../../experiments' import { getColorScale, truncateVerticalTitle } from '../vega/util' import { definedAndNonEmpty, reorderObjectList } from '../../util/array' import { removeMissingKeysFromObject } from '../../util/object' @@ -39,10 +38,9 @@ import { MultiSourceEncoding, MultiSourceVariations } from '../multiSource/collect' +import { SelectedExperimentWithColor } from '../../experiments/model' export class PlotsModel extends ModelWithPersistence { - private readonly experiments: Experiments - private plotSizes: Record private sectionCollapsed: SectionCollapsed private branchRevisions: Record = {} @@ -62,13 +60,8 @@ export class PlotsModel extends ModelWithPersistence { private selectedMetrics?: string[] private metricOrder: string[] - constructor( - dvcRoot: string, - experiments: Experiments, - workspaceState: Memento - ) { + constructor(dvcRoot: string, workspaceState: Memento) { super(dvcRoot, workspaceState) - this.experiments = experiments this.plotSizes = this.revive( PersistenceKey.PLOT_SIZES, @@ -86,10 +79,18 @@ export class PlotsModel extends ModelWithPersistence { this.metricOrder = this.revive(PersistenceKey.PLOT_METRIC_ORDER, []) } - public async transformAndSetExperiments(data: ExperimentsOutput) { + public async transformAndSetExperiments( + data: ExperimentsOutput, + branchRevisions: { + id: string + sha: string | undefined + }[], + revisions: string[], + hasCheckpoints = false + ) { const [checkpointPlots, workspaceRunningCheckpoint] = await Promise.all([ collectCheckpointPlotsData(data), - collectWorkspaceRunningCheckpoint(data, this.experiments.hasCheckpoints()) + collectWorkspaceRunningCheckpoint(data, hasCheckpoints) ]) if (!this.selectedMetrics && checkpointPlots) { @@ -101,15 +102,23 @@ export class PlotsModel extends ModelWithPersistence { this.setMetricOrder() - return this.removeStaleData() + return this.removeStaleData(branchRevisions, revisions) } - public async transformAndSetPlots(data: PlotsOutput, revs: string[]) { - const cliIdToLabel = this.getCLIIdToLabel() + public async transformAndSetPlots( + data: PlotsOutput, + newRevs: string[], + selectedRevs: SelectedExperimentWithColor[] + ) { + if (data.error) { + return + } + + const cliIdToLabel = this.getCLIIdToLabel(selectedRevs) this.fetchedRevs = new Set([ ...this.fetchedRevs, - ...revs.map(rev => cliIdToLabel[rev]) + ...newRevs.map(rev => cliIdToLabel[rev]) ]) const [{ comparisonData, revisionData }, templates, multiSourceVariations] = @@ -142,31 +151,34 @@ export class PlotsModel extends ModelWithPersistence { this.multiSourceVariations ) - this.setComparisonOrder() + this.setComparisonOrder(selectedRevs) this.deferred.resolve() } - public getCheckpointPlots() { + public getCheckpointPlots( + selectedExperiments: SelectedExperimentWithColor[] + ) { if (!this.checkpointPlots) { return } const colors = getColorScale( - this.experiments - .getSelectedExperiments() - .map(({ displayColor, id: revision }) => ({ displayColor, revision })) + selectedExperiments.map(({ displayColor, id: revision }) => ({ + displayColor, + revision + })) ) if (!colors) { return } - const { domain: selectedExperiments } = colors + const { domain } = colors return { colors, - plots: this.getPlots(this.checkpointPlots, selectedExperiments), + plots: this.getPlots(this.checkpointPlots, domain), selectedMetrics: this.getSelectedMetrics(), size: this.getPlotSize(Section.CHECKPOINT_PLOTS) } @@ -176,66 +188,60 @@ export class PlotsModel extends ModelWithPersistence { this.deleteRevisionData(id) } - public getUnfetchedRevisions() { - return this.getSelectedRevisions().filter( - revision => !this.fetchedRevs.has(revision) - ) - } - - public getMissingRevisions() { + public getMissingRevisions(selectedRevs: SelectedExperimentWithColor[]) { const cachedRevisions = new Set([ ...Object.keys(this.comparisonData), ...Object.keys(this.revisionData) ]) - return this.getSelectedRevisions() + return this.getSelectedRevisions(selectedRevs) .filter(label => !cachedRevisions.has(label)) .map(label => this.getCLIId(label)) } - public getMutableRevisions() { - return this.experiments.getMutableRevisions() + public getRevisionColors(selectedRevs: SelectedExperimentWithColor[]) { + return getColorScale(this.getSelectedRevisionDetails(selectedRevs)) } - public getRevisionColors() { - return getColorScale(this.getSelectedRevisionDetails()) - } - - public getSelectedRevisionDetails() { + public getSelectedRevisionDetails( + selectedRevs: SelectedExperimentWithColor[] + ) { return reorderObjectList( this.comparisonOrder, - this.experiments - .getSelectedRevisions() - .map(({ label, displayColor, logicalGroupName, id }) => ({ - displayColor, - group: logicalGroupName, - id, - revision: label - })), + selectedRevs.map(({ label, displayColor, logicalGroupName, id }) => ({ + displayColor, + group: logicalGroupName, + id, + revision: label + })), 'revision' ) } - public getTemplatePlots(order: TemplateOrder | undefined) { + public getTemplatePlots( + order: TemplateOrder | undefined, + selectedRevs: SelectedExperimentWithColor[] + ) { if (!definedAndNonEmpty(order)) { return } - const selectedRevisions = this.getSelectedRevisions() - - if (!definedAndNonEmpty(selectedRevisions)) { + if (!definedAndNonEmpty(selectedRevs)) { return } - return this.getSelectedTemplatePlots(order, selectedRevisions) + return this.getSelectedTemplatePlots(order, selectedRevs) } - public getComparisonPlots(paths: string[] | undefined) { + public getComparisonPlots( + paths: string[] | undefined, + selectedRevs: SelectedExperimentWithColor[] + ) { if (!paths) { return } - const selectedRevisions = this.getSelectedRevisions() + const selectedRevisions = this.getSelectedRevisions(selectedRevs) if (!definedAndNonEmpty(selectedRevisions)) { return } @@ -243,8 +249,11 @@ export class PlotsModel extends ModelWithPersistence { return this.getSelectedComparisonPlots(paths, selectedRevisions) } - public setComparisonOrder(revisions: string[] = this.comparisonOrder) { - const currentRevisions = this.getSelectedRevisions() + public setComparisonOrder( + selectedRevs: SelectedExperimentWithColor[], + revisions: string[] = this.comparisonOrder + ) { + const currentRevisions = this.getSelectedRevisions(selectedRevs) this.comparisonOrder = revisions.filter(revision => currentRevisions.includes(revision) @@ -317,16 +326,24 @@ export class PlotsModel extends ModelWithPersistence { return this.multiSourceEncoding } - private removeStaleData() { + public getSelectedRevisions(selectedRevs: SelectedExperimentWithColor[]) { + return selectedRevs.map(({ label }) => label) + } + + private removeStaleData( + branchRevisions: { + id: string + sha: string | undefined + }[], + revisions: string[] + ) { return Promise.all([ - this.removeStaleBranches(), - this.removeStaleRevisions() + this.removeStaleBranches(branchRevisions), + this.removeStaleRevisions(revisions) ]) } - private removeStaleRevisions() { - const revisions = this.experiments.getRevisions() - + private removeStaleRevisions(revisions: string[]) { this.comparisonData = removeMissingKeysFromObject( revisions, this.comparisonData @@ -344,10 +361,13 @@ export class PlotsModel extends ModelWithPersistence { } } - private removeStaleBranches() { - const currentBranchRevisions = collectBranchRevisionDetails( - this.experiments.getBranchRevisions() - ) + private removeStaleBranches( + branchRevisions: { + id: string + sha: string | undefined + }[] + ) { + const currentBranchRevisions = collectBranchRevisionDetails(branchRevisions) for (const id of Object.keys(this.branchRevisions)) { if (this.branchRevisions[id] !== currentBranchRevisions[id]) { this.deleteRevisionData(id) @@ -365,10 +385,10 @@ export class PlotsModel extends ModelWithPersistence { this.fetchedRevs.delete(id) } - private getCLIIdToLabel() { + private getCLIIdToLabel(selectedRevs: SelectedExperimentWithColor[]) { const mapping: { [shortSha: string]: string } = {} - for (const rev of this.getSelectedRevisions()) { + for (const rev of this.getSelectedRevisions(selectedRevs)) { mapping[this.getCLIId(rev)] = rev } @@ -379,10 +399,6 @@ export class PlotsModel extends ModelWithPersistence { return this.branchRevisions[label] || label } - private getSelectedRevisions() { - return this.experiments.getSelectedRevisions().map(({ label }) => label) - } - private getPlots( checkpointPlots: CheckpointPlot[], selectedExperiments: string[] @@ -439,15 +455,15 @@ export class PlotsModel extends ModelWithPersistence { private getSelectedTemplatePlots( order: TemplateOrder, - selectedRevisions: string[] + selectedRevs: SelectedExperimentWithColor[] ) { return collectSelectedTemplatePlots( order, - selectedRevisions, + this.getSelectedRevisions(selectedRevs), this.templates, this.revisionData, this.getPlotSize(Section.TEMPLATE_PLOTS), - this.getRevisionColors(), + this.getRevisionColors(selectedRevs), this.multiSourceEncoding ) } diff --git a/extension/src/plots/paths/model.ts b/extension/src/plots/paths/model.ts index 43d015fc0d..d27b7f9bcc 100644 --- a/extension/src/plots/paths/model.ts +++ b/extension/src/plots/paths/model.ts @@ -27,6 +27,10 @@ export class PathsModel extends PathSelectionModel { } public transformAndSet(data: PlotsOutput) { + if (data.error) { + return + } + const paths = collectPaths(this.data, data) this.setNewStatuses(paths) diff --git a/extension/src/plots/webview/messages.ts b/extension/src/plots/webview/messages.ts index cc268c9963..7d00ecb837 100644 --- a/extension/src/plots/webview/messages.ts +++ b/extension/src/plots/webview/messages.ts @@ -21,6 +21,7 @@ import { PathsModel } from '../paths/model' import { BaseWebview } from '../../webview' import { getModifiedTime } from '../../fileSystem' import { definedAndNonEmpty } from '../../util/array' +import { SelectedExperimentWithColor } from '../../experiments/model' export class WebviewMessages { private readonly paths: PathsModel @@ -47,21 +48,18 @@ export class WebviewMessages { this.updateData = updateData } - public sendWebviewMessage() { + public sendWebviewMessage( + selectedRevs: SelectedExperimentWithColor[], + selectedExperiments: SelectedExperimentWithColor[] + ) { this.getWebview()?.show({ - checkpoint: this.getCheckpointPlots(), - comparison: this.getComparisonPlots(), + checkpoint: this.getCheckpointPlots(selectedExperiments), + comparison: this.getComparisonPlots(selectedRevs), hasPlots: !!this.paths?.hasPaths(), hasSelectedPlots: definedAndNonEmpty(this.paths.getSelected()), sectionCollapsed: this.plots.getSectionCollapsed(), - selectedRevisions: this.plots.getSelectedRevisionDetails(), - template: this.getTemplatePlots() - }) - } - - public sendCheckpointPlotsMessage() { - this.getWebview()?.show({ - checkpoint: this.getCheckpointPlots() + selectedRevisions: this.plots.getSelectedRevisionDetails(selectedRevs), + template: this.getTemplatePlots(selectedRevs) }) } @@ -74,11 +72,20 @@ export class WebviewMessages { case MessageFromWebviewType.TOGGLE_PLOTS_SECTION: return this.setSectionCollapsed(message.payload) case MessageFromWebviewType.REORDER_PLOTS_COMPARISON: - return this.setComparisonOrder(message.payload) + return this.setComparisonOrder( + this.experiments.getSelectedRevisions(), + message.payload + ) case MessageFromWebviewType.REORDER_PLOTS_COMPARISON_ROWS: - return this.setComparisonRowsOrder(message.payload) + return this.setComparisonRowsOrder( + this.experiments.getSelectedRevisions(), + message.payload + ) case MessageFromWebviewType.REORDER_PLOTS_TEMPLATES: - return this.setTemplateOrder(message.payload) + return this.setTemplateOrder( + this.experiments.getSelectedRevisions(), + message.payload + ) case MessageFromWebviewType.REORDER_PLOTS_METRICS: return this.setMetricOrder(message.payload) case MessageFromWebviewType.SELECT_PLOTS: @@ -96,6 +103,14 @@ export class WebviewMessages { } } + private sendCheckpointPlotsMessage() { + this.getWebview()?.show({ + checkpoint: this.getCheckpointPlots( + this.experiments.getSelectedExperiments() + ) + }) + } + private setSelectedMetrics(metrics: string[]) { this.plots.setSelectedMetrics(metrics) this.sendCheckpointPlotsAndEvent(EventName.VIEWS_PLOTS_METRICS_SELECTED) @@ -120,9 +135,12 @@ export class WebviewMessages { ) } - private setComparisonOrder(order: string[]) { - this.plots.setComparisonOrder(order) - this.sendComparisonPlots() + private setComparisonOrder( + selectedRevs: SelectedExperimentWithColor[], + order: string[] + ) { + this.plots.setComparisonOrder(selectedRevs, order) + this.sendComparisonPlots(selectedRevs) sendTelemetryEvent( EventName.VIEWS_PLOTS_REVISIONS_REORDERED, undefined, @@ -130,9 +148,12 @@ export class WebviewMessages { ) } - private setComparisonRowsOrder(order: string[]) { + private setComparisonRowsOrder( + selectedRevs: SelectedExperimentWithColor[], + order: string[] + ) { this.paths.setComparisonPathsOrder(order) - this.sendComparisonPlots() + this.sendComparisonPlots(selectedRevs) sendTelemetryEvent( EventName.VIEWS_PLOTS_COMPARISON_ROWS_REORDERED, undefined, @@ -140,9 +161,12 @@ export class WebviewMessages { ) } - private setTemplateOrder(order: PlotsTemplatesReordered) { + private setTemplateOrder( + selectedRevs: SelectedExperimentWithColor[], + order: PlotsTemplatesReordered + ) { this.paths.setTemplateOrder(order) - this.sendTemplatePlots() + this.sendTemplatePlots(selectedRevs) sendTelemetryEvent( EventName.VIEWS_REORDER_PLOTS_TEMPLATES, undefined, @@ -217,22 +241,22 @@ export class WebviewMessages { }) } - private sendComparisonPlots() { + private sendComparisonPlots(selectedRevs: SelectedExperimentWithColor[]) { this.getWebview()?.show({ - comparison: this.getComparisonPlots(), - selectedRevisions: this.plots?.getSelectedRevisionDetails() + comparison: this.getComparisonPlots(selectedRevs), + selectedRevisions: this.plots?.getSelectedRevisionDetails(selectedRevs) }) } - private sendTemplatePlots() { + private sendTemplatePlots(selectedRevs: SelectedExperimentWithColor[]) { this.getWebview()?.show({ - template: this.getTemplatePlots() + template: this.getTemplatePlots(selectedRevs) }) } - private getTemplatePlots() { + private getTemplatePlots(selectedRevs: SelectedExperimentWithColor[]) { const paths = this.paths?.getTemplateOrder() - const plots = this.plots?.getTemplatePlots(paths) + const plots = this.plots?.getTemplatePlots(paths, selectedRevs) if (!this.plots || !plots || isEmpty(plots)) { return null @@ -244,9 +268,9 @@ export class WebviewMessages { } } - private getComparisonPlots() { + private getComparisonPlots(selectedRevs: SelectedExperimentWithColor[]) { const paths = this.paths.getComparisonPaths() - const comparison = this.plots.getComparisonPlots(paths) + const comparison = this.plots.getComparisonPlots(paths, selectedRevs) if (!this.plots || !comparison || isEmpty(comparison)) { return null } @@ -284,7 +308,9 @@ export class WebviewMessages { } } - private getCheckpointPlots() { - return this.plots?.getCheckpointPlots() || null + private getCheckpointPlots( + selectedExperiments: SelectedExperimentWithColor[] + ) { + return this.plots?.getCheckpointPlots(selectedExperiments) || null } } diff --git a/extension/src/test/suite/experiments/model/tree.test.ts b/extension/src/test/suite/experiments/model/tree.test.ts index 53fba711ed..6ac002173e 100644 --- a/extension/src/test/suite/experiments/model/tree.test.ts +++ b/extension/src/test/suite/experiments/model/tree.test.ts @@ -294,7 +294,9 @@ suite('Experiments Tree Test Suite', () => { await buildPlots(disposable, plotsDiffFixture) await plots.showWebview() - const initiallySelectedRevisions = plotsModel.getSelectedRevisionDetails() + const initiallySelectedRevisions = plotsModel.getSelectedRevisionDetails( + experiments.getSelectedRevisions() + ) const setSelectionModeSpy = spy( ExperimentsModel.prototype, @@ -345,7 +347,9 @@ suite('Experiments Tree Test Suite', () => { 'auto-apply filters to experiment selection is not enabled when the user selects to use the most recent' ).to.be.false expect( - plotsModel.getSelectedRevisionDetails(), + plotsModel.getSelectedRevisionDetails( + experiments.getSelectedRevisions() + ), 'all running and the most recent experiments are now selected' ).to.deep.equal([ { @@ -396,10 +400,10 @@ suite('Experiments Tree Test Suite', () => { 'the missing revisions have been requested' ).to.be.calledWithExactly( dvcDemoPath, - '1ee5f2e', - '2173124', '23250b3', - 'd1343a8' + 'd1343a8', + '1ee5f2e', + '2173124' ) }).timeout(WEBVIEW_TEST_TIMEOUT) diff --git a/extension/src/test/suite/plots/data/index.test.ts b/extension/src/test/suite/plots/data/index.test.ts index f50fbdce40..2e7567197d 100644 --- a/extension/src/test/suite/plots/data/index.test.ts +++ b/extension/src/test/suite/plots/data/index.test.ts @@ -6,6 +6,7 @@ import { PlotsData } from '../../../../plots/data' import { PlotsModel } from '../../../../plots/model' import { dvcDemoPath } from '../../../util' import { buildDependencies } from '../../util' +import { Experiments } from '../../../../experiments' suite('Plots Data Test Suite', () => { const disposable = Disposable.fn() @@ -31,17 +32,25 @@ suite('Plots Data Test Suite', () => { const mockGetMissingRevisions = stub().returns(missingRevisions) const mockGetMutableRevisions = stub().returns(mutableRevisions) + const mockGetSelectedRevisions = stub().returns( + missingRevisions.map(rev => ({ label: rev })) + ) const mockPlotsModel = { - getMissingRevisions: mockGetMissingRevisions, - getMutableRevisions: mockGetMutableRevisions + getMissingRevisions: mockGetMissingRevisions } as unknown as PlotsModel + const mockExperiments = { + getMutableRevisions: mockGetMutableRevisions, + getSelectedRevisions: mockGetSelectedRevisions + } as unknown as Experiments + const data = disposable.track( new PlotsData( dvcDemoPath, internalCommands, mockPlotsModel, + mockExperiments, updatesPaused ) ) diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index 49313ef4ee..fb1a84bca5 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -89,11 +89,12 @@ suite('Plots Test Suite', () => { it('should call plots diff with new experiment revisions but not checkpoints', async () => { const mockNow = getMockNow() - const { mockPlotsDiff, data, experiments } = await buildPlots( + const { plots, mockPlotsDiff, experiments } = await buildPlots( disposable, plotsDiffFixture ) mockPlotsDiff.resetHistory() + await plots.showWebview() const updatedExpShowFixture = merge( cloneDeep(expShowFixtureWithoutErrors), @@ -116,7 +117,9 @@ suite('Plots Test Suite', () => { ) const dataUpdateEvent = new Promise(resolve => - disposable.track(data.onDidUpdate(() => resolve(undefined))) + disposable.track( + experiments.onDidChangeExperiments(() => resolve(undefined)) + ) ) bypassProcessManagerDebounce(mockNow) @@ -129,11 +132,21 @@ suite('Plots Test Suite', () => { }) it('should call plots diff with the branch name whenever the current branch commit changes', async () => { - const mockNow = getMockNow() - const { data, experiments, mockPlotsDiff } = await buildPlots( - disposable, - plotsDiffFixture + const { plots, experiments, mockPlotsDiff, webviewMessages } = + await buildPlots(disposable, plotsDiffFixture) + + const mockSendPlots = stub(webviewMessages, 'sendWebviewMessage') + + const plotsSentEvent = new Promise(resolve => + mockSendPlots.callsFake(() => { + resolve(undefined) + }) ) + await plots.showWebview() + + await plotsSentEvent + mockSendPlots.resetBehavior() + mockPlotsDiff.resetHistory() const committedExperiment = { @@ -152,30 +165,38 @@ suite('Plots Test Suite', () => { workspace: committedExperiment } - const dataUpdateEvent = new Promise(resolve => - disposable.track(data.onDidUpdate(() => resolve(undefined))) + const plotsResent = new Promise(resolve => + mockSendPlots.callsFake(() => { + resolve(undefined) + }) ) - bypassProcessManagerDebounce(mockNow) experiments.setState(updatedExpShowFixture) - await dataUpdateEvent + await plotsResent expect(mockPlotsDiff).to.be.calledOnce expect(mockPlotsDiff).to.be.calledWithExactly( dvcDemoPath, - '9235a02', - 'workspace' + 'workspace', + '9235a02' ) }) it('should re-fetch data when moving between branches', async () => { - const mockNow = getMockNow() - const { data, experiments, mockPlotsDiff, plotsModel, webviewMessages } = + const { plots, experiments, mockPlotsDiff, plotsModel, webviewMessages } = await buildPlots(disposable, plotsDiffFixture) - mockPlotsDiff.resetHistory() - const mockSendPlots = stub(webviewMessages, 'sendWebviewMessage') + const plotsSentEvent = new Promise(resolve => + mockSendPlots.callsFake(() => { + resolve(undefined) + }) + ) + await plots.showWebview() + await plotsSentEvent + mockSendPlots.resetBehavior() + + mockPlotsDiff.resetHistory() mockPlotsDiff .onFirstCall() @@ -214,50 +235,58 @@ suite('Plots Test Suite', () => { const branchChangedEvent = new Promise(resolve => disposable.track( - data.onDidUpdate(() => { + experiments.onDidChangeExperiments(() => { if (plotsModel) { resolve(undefined) } }) ) ) - const plotsSentEvent = new Promise(resolve => + const plotsResentEvent = new Promise(resolve => mockSendPlots.callsFake(() => { - if (isEqual(plotsModel.getMissingRevisions(), ['9235a02'])) { + if ( + isEqual( + plotsModel.getMissingRevisions( + experiments.getSelectedRevisions() + ), + ['9235a02'] + ) + ) { resolve(undefined) } }) ) - bypassProcessManagerDebounce(mockNow) experiments.setState(updatedExpShowFixture) await branchChangedEvent - await plotsSentEvent + await plotsResentEvent + mockSendPlots.resetBehavior() expect(mockPlotsDiff).to.be.calledOnce expect(mockPlotsDiff).to.be.calledWithExactly( dvcDemoPath, - '9235a02', - 'workspace' + 'workspace', + '9235a02' ) - bypassProcessManagerDebounce(mockNow, 2) - const dataUpdateEvent = new Promise(resolve => - disposable.track(data.onDidUpdate(() => resolve(undefined))) + const originalPlotsSentEvent = new Promise(resolve => + mockSendPlots.callsFake(() => { + resolve(undefined) + }) ) experiments.setState(expShowFixtureWithoutErrors) - await dataUpdateEvent + await originalPlotsSentEvent expect(mockPlotsDiff).to.be.calledTwice expect(mockPlotsDiff).to.be.calledWithExactly( dvcDemoPath, - '1ba7bcd', - '42b8736', - '4fb124a', + 'workspace', '53c3851', - 'workspace' + '4fb124a', + '42b8736', + '1ba7bcd' ) }).timeout(WEBVIEW_TEST_TIMEOUT) @@ -391,7 +420,7 @@ suite('Plots Test Suite', () => { }).timeout(WEBVIEW_TEST_TIMEOUT) it('should handle a comparison revisions reordered message from the webview', async () => { - const { plots, plotsModel, messageSpy } = await buildPlots( + const { experiments, plots, plotsModel, messageSpy } = await buildPlots( disposable, plotsDiffFixture ) @@ -418,6 +447,7 @@ suite('Plots Test Suite', () => { expect(mockSetComparisonOrder).to.be.calledOnce expect(mockSetComparisonOrder).to.be.calledWithExactly( + experiments.getSelectedRevisions(), mockComparisonOrder ) expect(messageSpy).to.be.calledOnce @@ -766,17 +796,28 @@ suite('Plots Test Suite', () => { const { experiments, plots, messageSpy, mockPlotsDiff } = await buildPlots(disposable, multiSourcePlotsDiffFixture) + messageSpy.restore() + stub(experiments, 'getSelectedRevisions').returns([ { label: 'workspace' }, { label: 'main' } ] as SelectedExperimentWithColor[]) const webview = await plots.showWebview() + const mockShow = stub(webview, 'show') + const messageReceived = new Promise(resolve => + mockShow.callsFake(() => { + resolve(undefined) + return Promise.resolve(true) + }) + ) + await webview.isReady() + await messageReceived expect(mockPlotsDiff).to.be.called - const { template: templateData } = getFirstArgOfLastCall(messageSpy) + const { template: templateData } = getFirstArgOfLastCall(mockShow) const [singleViewSection, multiViewSection] = ( templateData as TemplatePlotsData