Skip to content

Add tests for column depth configuration #2471

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 26, 2022
Merged

Add tests for column depth configuration #2471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fcba9ce
Add tests for table depth config
julieg18 Sep 26, 2022
b8b2300
Merge branch 'main' into add-tests-for-table-depth-config
julieg18 Sep 26, 2022
252020b
Use default value in getConfiguration
julieg18 Sep 26, 2022
3e27203
Merge branch 'main' into add-tests-for-table-depth-config
julieg18 Sep 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions extension/src/experiments/columns/collect/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,13 @@ import columnsFixture from '../../../test/fixtures/expShow/columns'
import workspaceChangesFixture from '../../../test/fixtures/expShow/workspaceChanges'
import uncommittedDepsFixture from '../../../test/fixtures/expShow/uncommittedDeps'
import { ExperimentsOutput } from '../../../cli/dvc/reader'
import { getConfigValue } from '../../../vscode/config'

jest.mock('../../../vscode/config')

const mockedGetConfigValue = jest.mocked(getConfigValue)
mockedGetConfigValue.mockImplementation(() => 5)
Copy link
Contributor

@mattseddon mattseddon Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[I] The usual pattern is to use beforeEach with resetAllMocks to keep tests independent

beforeEach(() => {
  jest.resetAllMocks()
})

I thin you could use mockReturnValueOnce instead of mockImplementation as well. Final code would probably be

beforeEach(() => {
  jest.resetAllMocks()
  mockedGetConfigValue.mockReturnValueOnce(5)
})

for this particular file as we are not varying the option.


describe('collectColumns', () => {
it('should return a value equal to the columns fixture when given the output fixture', () => {
const columns = collectColumns(outputFixture)
Expand Down
14 changes: 8 additions & 6 deletions extension/src/experiments/columns/collect/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,24 @@
import { Column, ColumnType } from '../../webview/contract'
import { Value } from '../../../cli/dvc/reader'
import { ConfigKey, getConfigValue } from '../../../vscode/config'
import { INITIAL_TABLE_HEAD_MAX_LAYERS } from '../consts'

export type ColumnAccumulator = Record<string, Column>

export const limitAncestorDepth = (ancestors: string[], sep: string) => {
export const limitAncestorDepth = (
ancestors: string[],
sep: string,
limit = 5
) => {
const [path, ...rest] = ancestors
/*
The depth is only limited for the middle of the path array.
The first and final layer are excluded, and the
concatenated layer itself counts as one; because of this, we must subtract 3
from what we want the final layer count to be.
*/
const collectedLimit =
Number(getConfigValue(ConfigKey.EXP_TABLE_HEAD_MAX_LAYERS)) ||
INITIAL_TABLE_HEAD_MAX_LAYERS

const collectedLimit = Number(
getConfigValue(ConfigKey.EXP_TABLE_HEAD_MAX_LAYERS, limit)
)
const convertedLimit = (collectedLimit >= 3 ? collectedLimit : 3) - 3
if (rest.length <= convertedLimit) {
return ancestors
Expand Down
1 change: 0 additions & 1 deletion extension/src/experiments/columns/consts.ts
View file
Open in desktop

This file was deleted.

34 changes: 33 additions & 1 deletion extension/src/experiments/columns/model.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,30 +9,62 @@ import outputFixture from '../../test/fixtures/expShow/output'
import columnsFixture from '../../test/fixtures/expShow/columns'
import {
deeplyNestedOutput,
columns as deeplyNestedColumns
columns as deeplyNestedColumns,
columnsWithDepthOf10 as deeplyNestedColumnsWithDepthOf10,
columnsWithDepthOf3 as deeplyNestedColumnsWithDepthOf3
} from '../../test/fixtures/expShow/deeplyNested'
import {
dataTypesOutput,
columns as dataTypesColumns
} from '../../test/fixtures/expShow/dataTypes'
import { getConfigValue } from '../../vscode/config'

jest.mock('../../vscode/config')

const mockedGetConfigValue = jest.mocked(getConfigValue)
mockedGetConfigValue.mockImplementation(() => 5)
Copy link
Contributor

@mattseddon mattseddon Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[I] Can also use a beforeEach this time I would but the mockReturnValueOnce calls inside of the tests as the value is being varied throughout the file


describe('ColumnsModel', () => {
const exampleDvcRoot = 'test'

it('should return the expected columns when given the default output fixture', async () => {
const model = new ColumnsModel('', buildMockMemento())
await model.transformAndSet(outputFixture)
expect(mockedGetConfigValue).toHaveBeenCalled()
expect(model.getSelected()).toStrictEqual(columnsFixture)
})

it('should return the expected columns when given the deeply nested output fixture', async () => {
const model = new ColumnsModel('', buildMockMemento())
await model.transformAndSet(deeplyNestedOutput)
expect(mockedGetConfigValue).toHaveBeenCalled()
expect(model.getSelected()).toStrictEqual(deeplyNestedColumns)
})

it('should return the expected columns when the max depth config is set to 10', async () => {
mockedGetConfigValue.mockImplementation(() => 10)
const model = new ColumnsModel('', buildMockMemento())
await model.transformAndSet(deeplyNestedOutput)
expect(mockedGetConfigValue).toHaveBeenCalled()
expect(model.getSelected()).toStrictEqual(deeplyNestedColumnsWithDepthOf10)
})

it('should return the expected columns when the max depth config is set to 3', async () => {
mockedGetConfigValue.mockImplementation(() => 3)
const model = new ColumnsModel('', buildMockMemento())
await model.transformAndSet(deeplyNestedOutput)
expect(mockedGetConfigValue).toHaveBeenCalled()
expect(model.getSelected()).toStrictEqual(deeplyNestedColumnsWithDepthOf3)
})

it('should return the expected columns when the max depth config is set to -1', async () => {
mockedGetConfigValue.mockImplementation(() => -1)
const model = new ColumnsModel('', buildMockMemento())
await model.transformAndSet(deeplyNestedOutput)
expect(mockedGetConfigValue).toHaveBeenCalled()
expect(model.getSelected()).toStrictEqual(deeplyNestedColumnsWithDepthOf3)
})

it('should return the expected columns when given the data types output fixture', async () => {
const model = new ColumnsModel('', buildMockMemento())
await model.transformAndSet(dataTypesOutput)
Expand Down
261 changes: 261 additions & 0 deletions extension/src/test/fixtures/expShow/deeplyNested.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,267 @@ export const columns: Column[] = [
}
]

export const columnsWithDepthOf10: Column[] = [
timestampColumn,
{
hasChildren: true,
label: 'params.yaml',
parentPath: 'params',
path: 'params:params.yaml',
type: ColumnType.PARAMS
},
{
hasChildren: true,
label: 'nested1',
parentPath: 'params:params.yaml',
path: 'params:params.yaml:nested1',
type: ColumnType.PARAMS
},
{
hasChildren: false,
label: 'doubled',
maxStringLength: 15,
parentPath: 'params:params.yaml:nested1',
path: 'params:params.yaml:nested1.doubled',
pathArray: ['params', 'params.yaml', 'nested1', 'doubled'],
type: ColumnType.PARAMS,
types: ['string']
},
{
hasChildren: true,
label: 'nested2',
parentPath: 'params:params.yaml:nested1',
path: 'params:params.yaml:nested1.nested2',
type: ColumnType.PARAMS
},

{
hasChildren: true,
label: 'nested3',
parentPath: 'params:params.yaml:nested1.nested2',
path: 'params:params.yaml:nested1.nested2.nested3',
type: ColumnType.PARAMS
},
{
hasChildren: true,
label: 'nested4',
parentPath: 'params:params.yaml:nested1.nested2.nested3',
path: 'params:params.yaml:nested1.nested2.nested3.nested4',
type: ColumnType.PARAMS
},
{
hasChildren: true,
label: 'nested5',
parentPath: 'params:params.yaml:nested1.nested2.nested3.nested4',
path: 'params:params.yaml:nested1.nested2.nested3.nested4.nested5',
type: ColumnType.PARAMS
},
{
hasChildren: true,
label: 'nested6',
parentPath: 'params:params.yaml:nested1.nested2.nested3.nested4.nested5',
path: 'params:params.yaml:nested1.nested2.nested3.nested4.nested5.nested6',
type: ColumnType.PARAMS
},
{
hasChildren: false,
label: 'nested7',
maxStringLength: 6,
parentPath:
'params:params.yaml:nested1.nested2.nested3.nested4.nested5.nested6',
path: 'params:params.yaml:nested1.nested2.nested3.nested4.nested5.nested6.nested7',
pathArray: [
'params',
'params.yaml',
'nested1',
'nested2',
'nested3',
'nested4',
'nested5',
'nested6',
'nested7'
],
type: ColumnType.PARAMS,
types: ['string']
},
{
hasChildren: true,
label: 'nested5b',
parentPath: 'params:params.yaml:nested1.nested2.nested3.nested4',
path: 'params:params.yaml:nested1.nested2.nested3.nested4.nested5b',
type: ColumnType.PARAMS
},
{
hasChildren: false,
label: 'nested6',
maxStringLength: 23,
parentPath: 'params:params.yaml:nested1.nested2.nested3.nested4.nested5b',
path: 'params:params.yaml:nested1.nested2.nested3.nested4.nested5b.nested6',
pathArray: [
'params',
'params.yaml',
'nested1',
'nested2',
'nested3',
'nested4',
'nested5b',
'nested6'
],
type: ColumnType.PARAMS,
types: ['string']
},
{
hasChildren: false,
label: 'doubled',
maxStringLength: 16,
parentPath: 'params:params.yaml:nested1.nested2.nested3.nested4.nested5b',
path: 'params:params.yaml:nested1.nested2.nested3.nested4.nested5b.doubled',
pathArray: [
'params',
'params.yaml',
'nested1',
'nested2',
'nested3',
'nested4',
'nested5b',
'doubled'
],
type: ColumnType.PARAMS,
types: ['string']
},
{
hasChildren: false,
label: 'outlier',
maxStringLength: 1,
parentPath: 'params:params.yaml',
path: 'params:params.yaml:outlier',
pathArray: ['params', 'params.yaml', 'outlier'],
type: ColumnType.PARAMS,
types: ['number'],
maxNumber: 1,
minNumber: 1
}
]

export const columnsWithDepthOf3: Column[] = [
timestampColumn,
{
hasChildren: true,
label: 'params.yaml',
parentPath: 'params',
path: 'params:params.yaml',
type: ColumnType.PARAMS
},
{
hasChildren: true,
label: 'nested1',
parentPath: 'params:params.yaml',
path: 'params:params.yaml:nested1',
type: ColumnType.PARAMS
},

{
hasChildren: false,
label: 'doubled',
maxStringLength: 15,
parentPath: 'params:params.yaml:nested1',
path: 'params:params.yaml:nested1.doubled',
pathArray: ['params', 'params.yaml', 'nested1', 'doubled'],
type: ColumnType.PARAMS,
types: ['string']
},
{
hasChildren: true,
label: 'nested1.nested2.nested3.nested4.nested5.nested6',
parentPath: 'params:params.yaml',
path: 'params:params.yaml:nested1%2Enested2%2Enested3%2Enested4%2Enested5%2Enested6',
type: ColumnType.PARAMS
},
{
hasChildren: false,
label: 'nested7',
maxStringLength: 6,
parentPath:
'params:params.yaml:nested1%2Enested2%2Enested3%2Enested4%2Enested5%2Enested6',
path: 'params:params.yaml:nested1%2Enested2%2Enested3%2Enested4%2Enested5%2Enested6.nested7',
pathArray: [
'params',
'params.yaml',
'nested1',
'nested2',
'nested3',
'nested4',
'nested5',
'nested6',
'nested7'
],
type: ColumnType.PARAMS,
types: ['string']
},

{
hasChildren: true,
label: 'nested1.nested2.nested3.nested4.nested5b',
parentPath: 'params:params.yaml',
path: 'params:params.yaml:nested1%2Enested2%2Enested3%2Enested4%2Enested5b',
type: ColumnType.PARAMS
},
{
hasChildren: false,
label: 'nested6',
maxStringLength: 23,
parentPath:
'params:params.yaml:nested1%2Enested2%2Enested3%2Enested4%2Enested5b',
path: 'params:params.yaml:nested1%2Enested2%2Enested3%2Enested4%2Enested5b.nested6',
pathArray: [
'params',
'params.yaml',
'nested1',
'nested2',
'nested3',
'nested4',
'nested5b',
'nested6'
],
type: ColumnType.PARAMS,
types: ['string']
},

{
hasChildren: false,
label: 'doubled',
maxStringLength: 16,
parentPath:
'params:params.yaml:nested1%2Enested2%2Enested3%2Enested4%2Enested5b',
path: 'params:params.yaml:nested1%2Enested2%2Enested3%2Enested4%2Enested5b.doubled',
pathArray: [
'params',
'params.yaml',
'nested1',
'nested2',
'nested3',
'nested4',
'nested5b',
'doubled'
],
type: ColumnType.PARAMS,
types: ['string']
},

{
hasChildren: false,
label: 'outlier',
maxStringLength: 1,
parentPath: 'params:params.yaml',
path: 'params:params.yaml:outlier',
pathArray: ['params', 'params.yaml', 'outlier'],
type: ColumnType.PARAMS,
types: ['number'],
maxNumber: 1,
minNumber: 1
}
]

export const rows = [
{
id: 'workspace',
Expand Down
7 changes: 5 additions & 2 deletions extension/src/vscode/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,11 @@ export enum ConfigKey {
PYTHON_PATH = 'dvc.pythonPath'
}

export const getConfigValue = <T = string>(key: ConfigKey): T =>
workspace.getConfiguration().get(key, '') as unknown as T
export const getConfigValue = <T = string, D = string>(
key: ConfigKey,
defaultValue?: D | T
Copy link
Contributor

@mattseddon mattseddon Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this translates to defaultValue?: string ... 😕

Copy link
Contributor

@mattseddon mattseddon Sep 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be

export const getConfigValue = <T = string>(
  key: ConfigKey,
  defaultValue: T | undefined
): T =>
  workspace.getConfiguration().get(key, defaultValue ?? '') as unknown as T

): T =>
workspace.getConfiguration().get(key, defaultValue ?? '') as unknown as T

export const setConfigValue = (key: ConfigKey, value: unknown) =>
workspace.getConfiguration().update(key, value)
Expand Down