Skip to content
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.

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

Clean up setup internals #3435

Merged
merged 13 commits into from
Mar 12, 2023
53 changes: 52 additions & 1 deletion extension/src/cli/dvc/reader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { getProcessEnv } from '../../env'
import expShowFixture from '../../test/fixtures/expShow/base/output'
import plotsDiffFixture from '../../test/fixtures/plotsDiff/output/minimal'
import { Config } from '../../config'
import { joinEnvPath } from '../../util/env'

jest.mock('vscode')
jest.mock('@hediet/std/disposable')
Expand All @@ -31,6 +32,8 @@ const mockedEnv = {
}
const JSON_FLAG = '--json'

const mockedGetPythonBinPath = jest.fn()

beforeEach(() => {
jest.resetAllMocks()
mockedGetProcessEnv.mockReturnValueOnce(mockedEnv)
Expand All @@ -49,7 +52,7 @@ describe('CliReader', () => {
const dvcReader = new DvcReader(
{
getCliPath: () => undefined,
getPythonBinPath: () => undefined
getPythonBinPath: mockedGetPythonBinPath
} as unknown as Config,
{
processCompleted: {
Expand Down Expand Up @@ -144,6 +147,33 @@ describe('CliReader', () => {
})
})

describe('globalVersion', () => {
it('should call execute process with the correct parameters (does not respect pythonBinPath)', async () => {
const cwd = __dirname
const stdout = '3.9.11'
mockedCreateProcess.mockReturnValueOnce(getMockedProcess(stdout))
mockedGetPythonBinPath.mockReturnValueOnce('python')
const output = await dvcReader.globalVersion(cwd)

expect(output).toStrictEqual(stdout)
expect(mockedCreateProcess).toHaveBeenCalledWith({
args: ['--version'],
cwd,
env: mockedEnv,
executable: 'dvc'
})
})

it('should not retry if the process fails (cannot find cli - extension should reset)', async () => {
const cwd = __dirname
mockedCreateProcess.mockImplementationOnce(() => {
throw new Error('spawn dvc ENOENT retrying...')
})

await expect(dvcReader.globalVersion(cwd)).rejects.toBeTruthy()
})
})

describe('plotsDiff', () => {
it('should handle empty output being returned', async () => {
const cwd = __dirname
Expand Down Expand Up @@ -229,6 +259,27 @@ describe('CliReader', () => {
})
})

it('should respect the pythonBinPath parameters', async () => {
const cwd = __dirname
const stdout = '2.47.0'
const mockedPythonPath = 'some/python/path'
const mockedPythonBinPath = [mockedPythonPath, 'python'].join('/')
mockedGetPythonBinPath.mockReturnValue(mockedPythonBinPath)
mockedCreateProcess.mockReturnValueOnce(getMockedProcess(stdout))
const output = await dvcReader.version(cwd)

expect(output).toStrictEqual(stdout)
expect(mockedCreateProcess).toHaveBeenCalledWith({
args: ['-m', 'dvc', '--version'],
cwd,
env: {
...mockedEnv,
PATH: joinEnvPath(mockedPythonPath, mockedEnv.PATH)
},
executable: mockedPythonBinPath
})
})

it('should not retry if the process fails (cannot find cli - extension should reset)', async () => {
const cwd = __dirname
mockedCreateProcess.mockImplementationOnce(() => {
Expand Down
13 changes: 10 additions & 3 deletions extension/src/cli/dvc/reader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,11 @@ export const isDvcError = <
export const autoRegisteredCommands = {
DATA_STATUS: 'dataStatus',
EXP_SHOW: 'expShow',
GLOBAL_VERSION: 'globalVersion',
PLOTS_DIFF: 'plotsDiff',
STAGE_LIST: 'listStages'
ROOT: 'root',
STAGE_LIST: 'listStages',
VERSION: 'version'
} as const

export class DvcReader extends DvcCli {
Expand Down Expand Up @@ -104,9 +107,13 @@ export class DvcReader extends DvcCli {
} catch {}
}

public version(cwd: string, isCliGlobal?: true): Promise<string> {
public version(cwd: string): Promise<string> {
return this.executeDvcProcess(cwd, Flag.VERSION)
}

public globalVersion(cwd: string): Promise<string> {
const options = getOptions(
isCliGlobal ? undefined : this.config.getPythonBinPath(),
undefined,
this.config.getCliPath(),
cwd,
Flag.VERSION
Expand Down
3 changes: 2 additions & 1 deletion extension/src/cli/git/executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { getOptions } from './options'
import { typeCheckCommands } from '..'

export const autoRegisteredCommands = {
GIT_INIT: 'gitInit',
GIT_PUSH_BRANCH: 'pushBranch',
GIT_RESET_WORKSPACE: 'resetWorkspace',
GIT_STAGE_ALL: 'stageAll',
Expand All @@ -17,7 +18,7 @@ export class GitExecutor extends GitCli {
this
)

public init(cwd: string) {
public gitInit(cwd: string) {
const options = getOptions(cwd, Command.INITIALIZE)

return this.executeProcess(options)
Expand Down
1 change: 1 addition & 0 deletions extension/src/cli/git/reader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export const autoRegisteredCommands = {
GIT_GET_REMOTE_URL: 'getRemoteUrl',
GIT_GET_REPOSITORY_ROOT: 'getGitRepositoryRoot',
GIT_HAS_CHANGES: 'hasChanges',
GIT_HAS_NO_COMMITS: 'hasNoCommits',
GIT_LIST_UNTRACKED: 'listUntracked'
} as const

Expand Down
20 changes: 11 additions & 9 deletions extension/src/extension.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@ import { LanguageClient } from './languageClient'
import { collectRunningExperimentPids } from './experiments/processExecution/collect'
import { registerPatchCommand } from './patch'
import { DvcViewer } from './cli/dvc/viewer'
import { registerSetupCommands } from './setup/register'
import { Status } from './status'

export class Extension extends Disposable {
protected readonly internalCommands: InternalCommands

Expand Down Expand Up @@ -131,6 +134,8 @@ export class Extension extends Disposable {
new InternalCommands(outputChannel, ...clis)
)

const status = this.dispose.track(new Status(config, ...clis))
Copy link
Member Author

Choose a reason for hiding this comment

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

[F] There was a bug in the way that we were creating Status it did not contain the dvcViewer.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a type in a quick follow-up


this.experiments = this.dispose.track(
new WorkspaceExperiments(
this.internalCommands,
Expand Down Expand Up @@ -183,18 +188,14 @@ export class Extension extends Disposable {

this.setup = this.dispose.track(
new Setup(
stopWatch,
config,
this.dvcExecutor,
this.dvcReader,
this.dvcRunner,
this.gitExecutor,
this.gitReader,
() => this.initialize(),
() => this.resetMembers(),
this.experiments,
this.internalCommands,
this.experiments,
status,
this.resourceLocator.dvcIcon,
stopWatch,
() => this.initialize(),
() => this.resetMembers(),
() =>
collectWorkspaceScale(
this.getRoots(),
Expand All @@ -215,6 +216,7 @@ export class Extension extends Disposable {
this.connect
)
registerPlotsCommands(this.plots, this.internalCommands, this.setup)
registerSetupCommands(this.setup, this.internalCommands)
this.internalCommands.registerExternalCommand(
RegisteredCommands.EXPERIMENT_AND_PLOTS_SHOW,
async (context: VsCodeContext) => {
Expand Down
Loading