From 5c9bf0605f39702338b6db5a2d2addf36b6616ca Mon Sep 17 00:00:00 2001 From: Matt Seddon Date: Mon, 3 Apr 2023 10:42:39 +1000 Subject: [PATCH] Show revision level errors --- extension/src/cli/dvc/contract.ts | 2 +- extension/src/plots/errors/collect.test.ts | 166 +++++++++++++++++-- extension/src/plots/errors/collect.ts | 39 ++++- extension/src/plots/errors/model.ts | 16 +- extension/src/plots/index.ts | 3 +- extension/src/plots/paths/collect.ts | 3 + extension/src/test/suite/plots/index.test.ts | 4 +- 7 files changed, 209 insertions(+), 24 deletions(-) diff --git a/extension/src/cli/dvc/contract.ts b/extension/src/cli/dvc/contract.ts index 18d33a3fda..566acd46a0 100644 --- a/extension/src/cli/dvc/contract.ts +++ b/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 diff --git a/extension/src/plots/errors/collect.test.ts b/extension/src/plots/errors/collect.test.ts index 067a9fdcf9..55c0d34f2e 100644 --- a/extension/src/plots/errors/collect.test.ts +++ b/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))) + ) + }) }) diff --git a/extension/src/plots/errors/collect.ts b/extension/src/plots/errors/collect.ts index f173285a90..20d582b268 100644 --- a/extension/src/plots/errors/collect.ts +++ b/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, + 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 => { + const acc = new Set() + 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 +} diff --git a/extension/src/plots/errors/model.ts b/extension/src/plots/errors/model.ts index cdb7f5c713..512e836b11 100644 --- a/extension/src/plots/errors/model.ts +++ b/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() - 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) { diff --git a/extension/src/plots/index.ts b/extension/src/plots/index.ts index 4b7c9b7241..54cea8117f 100644 --- a/extension/src/plots/index.ts +++ b/extension/src/plots/index.ts @@ -139,8 +139,9 @@ export class Plots extends BaseRepository { 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() diff --git a/extension/src/plots/paths/collect.ts b/extension/src/plots/paths/collect.ts index 997c12a480..f20772c5a0 100644 --- a/extension/src/plots/paths/collect.ts +++ b/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) } diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index 16af64ece5..907ef0bb81 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/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([])) })