Show revision level plot errors

extension/src/cli/dvc/contract.ts

@@ -101,7 +101,7 @@ export interface PlotsData {
 }
 
 export type PlotError = {
-  name: string
+  name?: string
   rev: string
   source?: string
 } & ErrorContents

extension/src/plots/errors/collect.test.ts

@@ -1,5 +1,10 @@
 import { join } from 'path'
-import { collectErrors, collectImageErrors, collectPathErrors } from './collect'
+import {
+  collectErrorPaths,
+  collectErrors,
+  collectImageErrors,
+  collectPathErrors
+} from './collect'
 import { EXPERIMENT_WORKSPACE_ID } from '../../cli/dvc/contract'
 
 describe('collectErrors', () => {
@@ -180,7 +185,7 @@ describe('collectPathErrorsTable', () => {
   it('should return undefined if the errors do not relate to selected revisions', () => {
     const rev = 'main'
     const path = 'wat'
-    const markdownTable = collectPathErrors(
+    const errors = collectPathErrors(
       path,
       [EXPERIMENT_WORKSPACE_ID],
       [
@@ -201,13 +206,13 @@ describe('collectPathErrorsTable', () => {
         }
       ]
     )
-    expect(markdownTable).toBeUndefined()
+    expect(errors).toBeUndefined()
   })
 
   it('should return undefined if the errors do not relate to the path', () => {
     const rev = 'main'
     const path = 'wat'
-    const markdownTable = collectPathErrors(
+    const errors = collectPathErrors(
       join('other', 'path'),
       [rev],
       [
@@ -228,13 +233,13 @@ describe('collectPathErrorsTable', () => {
         }
       ]
     )
-    expect(markdownTable).toBeUndefined()
+    expect(errors).toBeUndefined()
   })
 
   it('should return an array of objects containing error messages that relate to the select revision and provided path', () => {
     const rev = 'a-really-long-branch-name'
     const path = 'wat'
-    const markdownTable = collectPathErrors(
+    const errors = collectPathErrors(
       path,
       [EXPERIMENT_WORKSPACE_ID, rev],
       [
@@ -260,14 +265,14 @@ describe('collectPathErrorsTable', () => {
         }
       ]
     )
-    expect(markdownTable).toStrictEqual([
+    expect(errors).toStrictEqual([
       { msg: 'wat not found.', rev: EXPERIMENT_WORKSPACE_ID },
       { msg: 'catastrophic error', rev },
       { msg: 'UNEXPECTEDERRRRROR', rev }
     ])
   })
 
-  it('should not duplicate entries in the table', () => {
+  it('should not duplicate entries', () => {
     const path = 'dvc.yaml::Accuracy'
     const msg =
       "Could not find provided field ('acc_') in data fields ('step, acc')."
@@ -279,7 +284,7 @@ describe('collectPathErrorsTable', () => {
       type
     }
 
-    const markdownTable = collectPathErrors(
+    const errors = collectPathErrors(
       'dvc.yaml::Accuracy',
       [EXPERIMENT_WORKSPACE_ID, 'main', 'test-plots-diff', 'aa1401b'],
       [
@@ -300,7 +305,7 @@ describe('collectPathErrorsTable', () => {
       ]
     )
 
-    expect(markdownTable).toStrictEqual([
+    expect(errors).toStrictEqual([
       {
         msg,
         rev: EXPERIMENT_WORKSPACE_ID
@@ -312,4 +317,145 @@ describe('collectPathErrorsTable', () => {
       { msg, rev: 'aa1401b' }
     ])
   })
+
+  it('should provide an entry for the path if there is a revision level error', () => {
+    const rev = 'main'
+    const path = 'wat'
+    const errors = collectPathErrors(
+      path,
+      [EXPERIMENT_WORKSPACE_ID, rev],
+      [
+        {
+          msg: `${path} not found.`,
+          path,
+          rev: EXPERIMENT_WORKSPACE_ID
+        },
+        {
+          msg: 'UNEXPECTEDERRRRROR',
+          path: undefined,
+          rev: EXPERIMENT_WORKSPACE_ID
+        },
+        {
+          msg: 'catastrophic error',
+          path,
+          rev
+        }
+      ]
+    )
+    expect(errors).toStrictEqual([
+      { msg: 'wat not found.', rev: EXPERIMENT_WORKSPACE_ID },
+      { msg: 'UNEXPECTEDERRRRROR', rev: EXPERIMENT_WORKSPACE_ID },
+      { msg: 'catastrophic error', rev }
+    ])
+  })
+})
+
+describe('collectErrorPaths', () => {
+  it('should apply a revision level error to all available paths if the revision is selected', () => {
+    const mockedDvcRoot = join('root', 'a')
+    const selectedRevisions = ['main', 'test-plots-diff', 'aa1401b']
+    const paths = ['dvc.yaml::Accuracy', 'dvc.yaml::Loss']
+    const errors = [
+      {
+        msg: "'./dvc.yaml' validation failed",
+        path: undefined,
+        rev: 'main',
+        type: 'YAMLValidationError'
+      }
+    ]
+
+    const errorPaths = collectErrorPaths(
+      mockedDvcRoot,
+      selectedRevisions,
+      paths,
+      errors
+    )
+
+    expect(errorPaths).toStrictEqual(
+      new Set(paths.map(path => join(mockedDvcRoot, path)))
+    )
+  })
+
+  it('should ignore revision level errors if the revision is not selected', () => {
+    const mockedDvcRoot = join('root', 'a')
+    const selectedRevisions = ['test-plots-diff', 'aa1401b']
+    const paths = ['dvc.yaml::Accuracy', 'dvc.yaml::Loss']
+    const errors = [
+      {
+        msg: "'./dvc.yaml' validation failed",
+        path: undefined,
+        rev: 'main',
+        type: 'YAMLValidationError'
+      }
+    ]
+
+    const errorPaths = collectErrorPaths(
+      mockedDvcRoot,
+      selectedRevisions,
+      paths,
+      errors
+    )
+
+    expect(errorPaths).toStrictEqual(new Set([]))
+  })
+
+  it('should collect appropriate errors and ignore revision level errors if the revision is not selected', () => {
+    const mockedDvcRoot = join('root', 'a')
+    const selectedRevisions = ['test-plots-diff', 'aa1401b']
+    const paths = ['dvc.yaml::Accuracy', 'dvc.yaml::Loss']
+    const errors = [
+      {
+        msg: "'./dvc.yaml' validation failed",
+        path: undefined,
+        rev: 'main',
+        type: 'YAMLValidationError'
+      },
+      {
+        msg: 'it broke',
+        path: paths[0],
+        rev: 'test-plots-diff',
+        type: 'UnexpectedError'
+      }
+    ]
+
+    const errorPaths = collectErrorPaths(
+      mockedDvcRoot,
+      selectedRevisions,
+      paths,
+      errors
+    )
+
+    expect(errorPaths).toStrictEqual(new Set([join(mockedDvcRoot, paths[0])]))
+  })
+
+  it('should collect revision level and individual errors together', () => {
+    const mockedDvcRoot = join('root', 'a')
+    const selectedRevisions = ['test-plots-diff', 'main']
+    const paths = ['dvc.yaml::Accuracy', 'dvc.yaml::Loss']
+    const errors = [
+      {
+        msg: "'./dvc.yaml' validation failed",
+        path: undefined,
+        rev: 'main',
+        type: 'YAMLValidationError'
+      },
+      {
+        msg: 'it broke',
+        path: paths[0],
+        rev: 'test-plots-diff',
+        type: 'UnexpectedError'
+      }
+    ]
+
+    const errorPaths = collectErrorPaths(
+      mockedDvcRoot,
+      selectedRevisions,
+      paths,
+      errors
+    )
+
+    expect(errorPaths).toStrictEqual(
+      new Set(paths.map(path => join(mockedDvcRoot, path)))
+    )
+  })
 })

extension/src/plots/errors/collect.ts

@@ -1,6 +1,7 @@
+import { join } from 'path'
 import { PlotError, PlotsOutput } from '../../cli/dvc/contract'
 
-export type Error = { path: string; rev: string; msg: string }
+export type Error = { path: string | undefined; rev: string; msg: string }
 
 const getMessage = (error: PlotError): string => {
   const { msg, type, source } = error
@@ -70,6 +71,8 @@ const isDuplicateError = (
       rev === existingRev && msg === existingMsg
   )
 
+const isRevisionError = (path: string | undefined): path is undefined => !path
+
 export const collectPathErrors = (
   path: string,
   selectedRevisions: string[],
@@ -79,7 +82,7 @@ export const collectPathErrors = (
   for (const error of errors) {
     const { msg, rev } = error
     if (
-      error.path !== path ||
+      (!isRevisionError(error.path) && error.path !== path) ||
       !selectedRevisions.includes(rev) ||
       isDuplicateError(acc, rev, msg)
     ) {
@@ -95,3 +98,35 @@ export const collectPathErrors = (
 
   return acc
 }
+
+const addErrorToEveryPath = (
+  acc: Set<string>,
+  dvcRoot: string,
+  paths: string[]
+) => {
+  for (const path of paths) {
+    acc.add(join(dvcRoot, path))
+  }
+}
+
+export const collectErrorPaths = (
+  dvcRoot: string,
+  selectedRevisions: string[],
+  paths: string[],
+  errors: Error[]
+): Set<string> => {
+  const acc = new Set<string>()
+  for (const { path, rev } of errors) {
+    if (!selectedRevisions.includes(rev)) {
+      continue
+    }
+
+    if (isRevisionError(path)) {
+      addErrorToEveryPath(acc, dvcRoot, paths)
+      continue
+    }
+
+    acc.add(join(dvcRoot, path))
+  }
+  return acc
+}

extension/src/plots/errors/model.ts

@@ -1,5 +1,6 @@
 import { join } from 'path'
 import {
+  collectErrorPaths,
   collectErrors,
   collectImageErrors,
   collectPathErrors,
@@ -42,18 +43,17 @@ export class ErrorsModel extends Disposable {
     return collectPathErrors(path, selectedRevisions, this.errors)
   }
 
-  public getErrorPaths(selectedRevisions: string[]) {
+  public getErrorPaths(selectedRevisions: string[], paths: string[]) {
     if (this.cliError) {
       return new Set([this.cliError.path])
     }
 
-    const acc = new Set<string>()
-    for (const { path, rev } of this.errors) {
-      if (selectedRevisions.includes(rev)) {
-        acc.add(join(this.dvcRoot, path))
-      }
-    }
-    return acc
+    return collectErrorPaths(
+      this.dvcRoot,
+      selectedRevisions,
+      paths,
+      this.errors
+    )
   }
 
   public getRevisionErrors(rev: string) {

extension/src/plots/index.ts

@@ -139,8 +139,9 @@ export class Plots extends BaseRepository<TPlotsData> {
   private notifyChanged() {
     const selectedRevisions = this.plots.getSelectedRevisions()
     this.paths.setSelectedRevisions(selectedRevisions)
+    const paths = this.paths.getTerminalNodes().map(({ path }) => path)
     this.decorationProvider.setState(
-      this.errors.getErrorPaths(selectedRevisions)
+      this.errors.getErrorPaths(selectedRevisions, paths)
     )
     this.pathsChanged.fire()
 

extension/src/plots/paths/collect.ts

@@ -223,6 +223,9 @@ const collectErrorPaths = (
 ) => {
   const paths = errors.map(({ name }) => name)
   for (const path of paths) {
+    if (!path) {
+      continue
+    }
     const revisions = collectErrorRevisions(path, errors, cliIdToLabel)
     acc = addRevisionsToPath(acc, data, path, revisions)
   }

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

@@ -1179,7 +1179,7 @@ suite('Plots Test Suite', () => {
       ])
 
       expect(
-        errorsModel.getErrorPaths(plotsModel.getSelectedRevisions()),
+        errorsModel.getErrorPaths(plotsModel.getSelectedRevisions(), []),
         'should return the correct path to give the item a DecorationError'
       ).to.deep.equal(new Set([errorItems[0].path]))
 
@@ -1196,7 +1196,7 @@ suite('Plots Test Suite', () => {
       ).to.deep.equal([])
 
       expect(
-        errorsModel.getErrorPaths(plotsModel.getSelectedRevisions()),
+        errorsModel.getErrorPaths(plotsModel.getSelectedRevisions(), []),
         'should no long provide decorations to the plots paths tree'
       ).to.deep.equal(new Set([]))
     })