Ensure unique list of experiment passed to the rest of the app (#3925)

* make combined list of experiments unique

* remove leftover method

* deduplicate experiments in collection

* add regression tests

Author: mattseddon

extension/src/experiments/index.ts

@@ -626,7 +626,7 @@ export class Experiments extends BaseRepository<TableData> {
     }
 
     return await pickExperiment(
-      this.experiments.getCombinedList(),
+      this.experiments.getUniqueList(),
       this.getFirstThreeColumnOrder(),
       Title.SELECT_BASE_EXPERIMENT
     )

extension/src/experiments/model/collect.test.ts

@@ -85,4 +85,47 @@ describe('collectExperiments', () => {
       tags: []
     })
   })
+
+  it('should not collect the same experiment twice', () => {
+    const main = {
+      experiments: [
+        {
+          name: 'campy-pall',
+          rev: '0b4b001dfaa8f2c4cd2a62238699131ab2c679ea'
+        },
+        {
+          name: 'shyer-stir',
+          rev: '450e672f0d8913517ab2ab443f5d87b34f308290'
+        }
+      ],
+      name: 'main',
+      rev: '61bed4ce8913eca7f73ca754d65bc5daad1520e2'
+    }
+
+    const expShowWithDuplicateCommits = generateTestExpShowOutput(
+      {},
+      main,
+      {
+        name: 'branchOffMainWithCommit',
+        rev: '351e42ace3cb6a3a853c65bef285e60748cc6341'
+      },
+      main
+    )
+
+    const { experimentsByCommit, commits } = collectExperiments(
+      expShowWithDuplicateCommits,
+      false,
+      ''
+    )
+
+    expect(commits.length).toStrictEqual(3)
+
+    const experiments = experimentsByCommit.get('main')
+
+    expect(experiments?.length).toStrictEqual(2)
+    expect(experiments?.map(({ id }) => id).sort()).toStrictEqual([
+      'campy-pall',
+      'shyer-stir'
+    ])
+  })
 })

extension/src/experiments/model/collect.ts

@@ -251,16 +251,25 @@ const collectExpRange = (
   const expState = revs[0]
 
   const { name, rev } = expState
-  const { branch, id } = baseline
+  const { id: baselineId } = baseline
 
   const label =
     rev === EXPERIMENT_WORKSPACE_ID
       ? EXPERIMENT_WORKSPACE_ID
       : shortenForLabel(rev)
 
+  const experimentId = name || label
+
+  if (
+    acc.experimentsByCommit
+      .get(baselineId)
+      ?.find(({ id }) => id === experimentId)
+  ) {
+    return
+  }
+
   const experiment = transformExpState(
     {
-      branch,
       id: name || label,
       label
     },
@@ -275,7 +284,7 @@ const collectExpRange = (
   collectExecutorInfo(experiment, executor)
   collectRunningExperiment(acc, experiment)
 
-  addToMapArray(acc.experimentsByCommit, id, experiment)
+  addToMapArray(acc.experimentsByCommit, baselineId, experiment)
 }
 
 const setWorkspaceAsRunning = (

extension/src/experiments/model/index.test.ts

@@ -141,7 +141,7 @@ describe('ExperimentsModel', () => {
 
     model.transformAndSet(data, false, '')
 
-    const experiments = model.getCombinedList()
+    const experiments = model.getUniqueList()
 
     const changed: string[] = []
     for (const { deps, sha } of experiments) {
@@ -273,7 +273,7 @@ describe('ExperimentsModel', () => {
     model.setSelected([])
     expect(model.getSelectedRevisions().map(({ id }) => id)).toStrictEqual([])
 
-    model.setSelected(model.getCombinedList())
+    model.setSelected(model.getUniqueList())
     expect(model.getSelectedRevisions().map(({ id }) => id)).toStrictEqual([
       EXPERIMENT_WORKSPACE_ID,
       'testBranch',
@@ -358,4 +358,60 @@ describe('ExperimentsModel', () => {
       'three'
     ])
   })
+
+  it('should handle duplicate commits being returned in the data', () => {
+    const main = {
+      experiments: [
+        {
+          name: 'campy-pall',
+          rev: '0b4b001dfaa8f2c4cd2a62238699131ab2c679ea'
+        },
+        {
+          name: 'shyer-stir',
+          rev: '450e672f0d8913517ab2ab443f5d87b34f308290'
+        }
+      ],
+      name: 'main',
+      rev: '61bed4ce8913eca7f73ca754d65bc5daad1520e2'
+    }
+
+    const expShowWithDuplicateCommits = generateTestExpShowOutput(
+      {},
+      main,
+      {
+        name: 'branchOffMainWithCommit',
+        rev: '351e42ace3cb6a3a853c65bef285e60748cc6341'
+      },
+      main
+    )
+
+    const model = new ExperimentsModel('', buildMockMemento())
+    model.transformAndSet(expShowWithDuplicateCommits, false, '')
+    const distinctTreeItems = model.getWorkspaceAndCommits()
+    expect(distinctTreeItems).toHaveLength(3)
+
+    const tableRows = model.getRowData()
+    expect(tableRows).toHaveLength(4)
+    expect(tableRows.map(({ id }) => id)).toStrictEqual([
+      EXPERIMENT_WORKSPACE_ID,
+      'main',
+      'branchOffMainWithCommit',
+      'main'
+    ])
+
+    const tableExperimentRows = (
+      tableRows[1] as unknown as { subRows: Experiment[] }
+    ).subRows
+
+    const duplicateTableExperimentRows = (
+      tableRows[3] as unknown as { subRows: Experiment[] }
+    ).subRows
+
+    expect(tableExperimentRows).toHaveLength(2)
+    expect(tableExperimentRows.map(({ id }) => id)).toStrictEqual([
+      'campy-pall',
+      'shyer-stir'
+    ])
+    expect(tableExperimentRows).toStrictEqual(duplicateTableExperimentRows)
+  })
 })

extension/src/experiments/model/index.ts

@@ -248,11 +248,25 @@ export class ExperimentsModel extends ModelWithPersistence {
   }
 
   public getRevisionIds() {
-    return this.getCombinedList().map(({ id }) => id)
+    return this.getUniqueList().map(({ id }) => id)
   }
 
   public getSelectedRevisions() {
-    return this.getSelectedFromList(() => this.getCombinedList())
+    const acc: SelectedExperimentWithColor[] = []
+
+    for (const experiment of this.getUniqueList()) {
+      const { id } = experiment
+      const displayColor = this.coloredStatus[id]
+      if (displayColor) {
+        acc.push({ ...experiment, displayColor } as SelectedExperimentWithColor)
+      }
+    }
+
+    return copyOriginalColors()
+      .flatMap(orderedItem =>
+        acc.filter(item => item.displayColor === orderedItem)
+      )
+      .filter(Boolean)
   }
 
   public setSelected(selectedExperiments: Experiment[]) {
@@ -273,7 +287,7 @@ export class ExperimentsModel extends ModelWithPersistence {
   }
 
   public getLabels() {
-    return this.getCombinedList().map(({ label }) => label)
+    return this.getUniqueList().map(({ label }) => label)
   }
 
   public getLabelsToDecorate() {
@@ -283,7 +297,7 @@ export class ExperimentsModel extends ModelWithPersistence {
   }
 
   public getWorkspaceAndCommits() {
-    return [
+    const experiments = [
       {
         ...this.addDetails(this.workspace),
         hasChildren: false,
@@ -292,15 +306,18 @@ export class ExperimentsModel extends ModelWithPersistence {
         )
           ? ExperimentType.RUNNING
           : ExperimentType.WORKSPACE
-      },
-      ...this.commits.map(commit => {
-        return {
-          ...this.addDetails(commit),
-          hasChildren: !!this.experimentsByCommit.get(commit.id),
-          type: ExperimentType.COMMIT
-        }
-      })
+      }
     ]
+
+    for (const commit of this.getCommits()) {
+      experiments.push({
+        ...this.addDetails(commit),
+        hasChildren: !!this.experimentsByCommit.get(commit.id),
+        type: ExperimentType.COMMIT
+      })
+    }
+
+    return experiments
   }
 
   public getWorkspaceCommitsAndExperiments() {
@@ -309,14 +326,14 @@ export class ExperimentsModel extends ModelWithPersistence {
 
   public getErrors() {
     return new Set(
-      this.getCombinedList()
+      this.getUniqueList()
         .filter(({ error }) => error)
         .map(({ label }) => label)
     )
   }
 
   public getExperimentParams(id: string) {
-    const params = this.getCombinedList().find(
+    const params = this.getUniqueList().find(
       experiment => experiment.id === id
     )?.params
 
@@ -330,7 +347,7 @@ export class ExperimentsModel extends ModelWithPersistence {
   }
 
   public getCommitsAndExperiments() {
-    return collectOrderedCommitsAndExperiments(this.commits, commit =>
+    return collectOrderedCommitsAndExperiments(this.getCommits(), commit =>
       this.getExperimentsByCommit(commit)
     )
   }
@@ -387,16 +404,24 @@ export class ExperimentsModel extends ModelWithPersistence {
   }
 
   public getExperimentCount() {
-    return sum([this.getExperimentsAndQueued().length, this.commits.length, 1])
+    return sum([
+      this.getExperimentsAndQueued().length,
+      this.getCommits().length,
+      1
+    ])
   }
 
   public getFilteredCount() {
     const filtered = this.getFilteredExperiments()
     return filtered.length
   }
 
-  public getCombinedList() {
-    return [this.workspace, ...this.commits, ...this.getExperimentsAndQueued()]
+  public getUniqueList() {
+    return [
+      this.workspace,
+      ...this.getCommits(),
+      ...this.getExperimentsAndQueued()
+    ]
   }
 
   public getExperimentsByCommitForTree(commit: Experiment) {
@@ -452,6 +477,20 @@ export class ExperimentsModel extends ModelWithPersistence {
     return this.currentSorts.findIndex(({ path }) => path === pathToRemove)
   }
 
+  private getCommits() {
+    const ids = new Set<string>()
+    const commits: Experiment[] = []
+    for (const commit of this.commits) {
+      const { id } = commit
+      if (ids.has(id)) {
+        continue
+      }
+      commits.push(commit)
+      ids.add(id)
+    }
+    return commits
+  }
+
   private getFilteredExperiments() {
     const acc: Experiment[] = []
 
@@ -467,7 +506,10 @@ export class ExperimentsModel extends ModelWithPersistence {
   private getExperimentsByCommit(commit: Experiment) {
     const experiments = this.experimentsByCommit
       .get(commit.id)
-      ?.map(experiment => this.addDetails(experiment))
+      ?.map(experiment => ({
+        ...this.addDetails(experiment),
+        branch: commit.branch
+      }))
     if (!experiments) {
       return
     }
@@ -607,23 +649,6 @@ export class ExperimentsModel extends ModelWithPersistence {
     return color
   }
 
-  private getSelectedFromList(getList: () => Experiment[]) {
-    const acc: SelectedExperimentWithColor[] = []
-
-    for (const experiment of getList()) {
-      const displayColor = this.coloredStatus[experiment.id]
-      if (displayColor) {
-        acc.push({ ...experiment, displayColor } as SelectedExperimentWithColor)
-      }
-    }
-
-    return copyOriginalColors()
-      .flatMap(orderedItem =>
-        acc.filter(item => item.displayColor === orderedItem)
-      )
-      .filter(Boolean)
-  }
-
   private reviveColoredStatus() {
     const uniqueStatus: ColoredStatus = {}
     const colors = new Set<Color>()

extension/src/experiments/webview/contract.ts

@@ -46,7 +46,7 @@ export type Experiment = {
   starred?: boolean
   status?: ExperimentStatus
   timestamp?: string | null
-  branch: string | undefined
+  branch?: string
 }
 
 export const isRunning = (status: ExperimentStatus | undefined): boolean =>

extension/src/experiments/webview/messages.ts

@@ -407,7 +407,7 @@ export class WebviewMessages {
 
   private setSelectedExperiments(ids: string[]) {
     const experiments = this.experiments
-      .getCombinedList()
+      .getUniqueList()
       .filter(({ id }) => ids.includes(id))
 
     this.experiments.setSelected(experiments)

extension/src/test/suite/experiments/index.test.ts

@@ -873,7 +873,7 @@ suite('Experiments Test Suite', () => {
       const queuedId = '90aea7f'
 
       const isExperimentSelected = (expId: string): boolean =>
-        !!experimentsModel.getCombinedList().find(({ id }) => id === expId)
+        !!experimentsModel.getUniqueList().find(({ id }) => id === expId)
           ?.selected
 
       expect(
@@ -1101,7 +1101,7 @@ suite('Experiments Test Suite', () => {
       const areExperimentsStarred = (expIds: string[]): boolean =>
         expIds
           .map(expId =>
-            experimentsModel.getCombinedList().find(({ id }) => id === expId)
+            experimentsModel.getUniqueList().find(({ id }) => id === expId)
           )
           .every(exp => exp?.starred)