Skip to content

Commit

Permalink
Warn user before trying to add a stage to their pipeline (#4869)
Browse files Browse the repository at this point in the history
  • Loading branch information
mattseddon authored Oct 23, 2023
1 parent ef3b3ea commit a5a40a9
Show file tree
Hide file tree
Showing 6 changed files with 95 additions and 39 deletions.
2 changes: 1 addition & 1 deletion extension/src/experiments/webview/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ export class WebviewMessages {
}

private addConfiguration() {
return this.pipeline.checkOrAddPipeline()
return this.pipeline.addPipeline()
}

private async setMaxTableHeadDepth() {
Expand Down
80 changes: 46 additions & 34 deletions extension/src/pipeline/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import { getInput, getValidInput } from '../vscode/inputBox'
import { Title } from '../vscode/title'
import { quickPickOne, quickPickOneOrInput } from '../vscode/quickPick'
import { pickFile } from '../vscode/resourcePicker'
import { Response } from '../vscode/response'
import { Modal } from '../vscode/modal'

export enum ScriptCommand {
JUPYTER = 'jupyter nbconvert --to notebook --inplace --execute',
Expand Down Expand Up @@ -105,13 +107,56 @@ export class Pipeline extends DeferredDisposable {
)
}

public checkOrAddPipeline() {
public async checkOrAddPipeline() {
if (this.model.hasPipeline()) {
return
}

const response = await Modal.showInformation(
'To continue you must add some configuration. Would you like to add a stage now?',
Response.YES
)

if (response !== Response.YES) {
return
}

return this.addPipeline()
}

public async addPipeline() {
const stageName = await this.askForStageName()
if (!stageName) {
return
}

const { trainingScript, command, enteredManually } =
await this.askForTrainingScript()
if (!trainingScript) {
return
}

const dataUpdated = new Promise(resolve => {
const listener = this.dispose.track(
this.data.onDidUpdate(() => {
resolve(undefined)
this.dispose.untrack(listener)
listener.dispose()
})
)
})

void findOrCreateDvcYamlFile(
this.dvcRoot,
trainingScript,
stageName,
command,
!enteredManually
)
void this.data.managedUpdate()
return dataUpdated
}

public async addDataSeriesPlot() {
const cwd = await this.getCwd()

Expand Down Expand Up @@ -149,39 +194,6 @@ export class Pipeline extends DeferredDisposable {
return this.deferred.resolve()
}

private async addPipeline() {
const stageName = await this.askForStageName()
if (!stageName) {
return
}

const { trainingScript, command, enteredManually } =
await this.askForTrainingScript()
if (!trainingScript) {
return
}

const dataUpdated = new Promise(resolve => {
const listener = this.dispose.track(
this.data.onDidUpdate(() => {
resolve(undefined)
this.dispose.untrack(listener)
listener.dispose()
})
)
})

void findOrCreateDvcYamlFile(
this.dvcRoot,
trainingScript,
stageName,
command,
!enteredManually
)
void this.data.managedUpdate()
return dataUpdated
}

private async askForStageName() {
return await getValidInput(
Title.ENTER_STAGE_NAME,
Expand Down
4 changes: 2 additions & 2 deletions extension/src/test/suite/experiments/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1523,14 +1523,14 @@ suite('Experiments Test Suite', () => {
}).timeout(WEBVIEW_TEST_TIMEOUT)

it('should handle a message to add a configuration', async () => {
const { mockMessageReceived, mockCheckOrAddPipeline } =
const { mockMessageReceived, mockAddPipeline } =
await buildExperimentsWebview({ disposer: disposable })

mockMessageReceived.fire({
type: MessageFromWebviewType.ADD_CONFIGURATION
})

expect(mockCheckOrAddPipeline).to.be.calledOnce
expect(mockAddPipeline).to.be.calledOnce
}).timeout(WEBVIEW_TEST_TIMEOUT)

it('should handle a message to show more commits', async () => {
Expand Down
2 changes: 2 additions & 0 deletions extension/src/test/suite/experiments/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export const buildExperiments = ({
internalCommands
})
const mockCheckOrAddPipeline = stub(pipeline, 'checkOrAddPipeline')
const mockAddPipeline = stub(pipeline, 'addPipeline')
const mockSelectBranches = stub().resolves(['main', 'other'])
const mockMemento = buildMockMemento({
[`${PersistenceKey.EXPERIMENTS_BRANCHES}${dvcRoot}`]: ['main'],
Expand Down Expand Up @@ -130,6 +131,7 @@ export const buildExperiments = ({
experimentsModel: (experiments as any).experiments as ExperimentsModel,
gitReader,
internalCommands,
mockAddPipeline,
mockCheckOrAddPipeline,
mockCheckSignalFile,
mockExpShow,
Expand Down
38 changes: 36 additions & 2 deletions extension/src/test/suite/pipeline/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,53 @@ suite('Pipeline Test Suite', () => {
})

describe('Pipeline', () => {
it('should not show a modal when addPipeline is called', async () => {
const mockInputBox = stub(window, 'showInputBox').resolves(undefined)
const { mockShowInformation, pipeline } = buildPipeline({
disposer: disposable,
dvcYamls: []
})
await pipeline.isReady()
await pipeline.addPipeline()
expect(mockShowInformation).not.to.have.been.calledOnce
expect(mockInputBox).to.have.been.calledOnce
})

it('should ask to create a stage and not return a cwd if there is no pipeline', async () => {
const mockInputBox = stub(window, 'showInputBox').resolves(undefined)
const { pipeline } = buildPipeline({
const { mockShowInformation, pipeline } = buildPipeline({
disposer: disposable,
dvcYamls: []
})
await pipeline.isReady()
const cwd = await pipeline.getCwd()
expect(mockInputBox, 'the user should be prompted to add a pipeline').to
expect(
mockShowInformation,
'the user should be asked whether or not they want to add a stage'
).to.have.been.calledOnce
expect(mockInputBox, 'the user should be prompted for a stage name').to
.have.been.calledOnce
expect(cwd, 'no cwd is returned').to.be.undefined
})

it('should not create a stage if the user does not want to add one', async () => {
const mockInputBox = stub(window, 'showInputBox').resolves(undefined)
const { mockShowInformation, pipeline } = buildPipeline({
disposer: disposable,
dvcYamls: []
})
mockShowInformation.resetBehavior()
mockShowInformation.resolves(undefined)
await pipeline.isReady()
const cwd = await pipeline.getCwd()
expect(
mockShowInformation,
'the user should be asked whether or not they want to add a stage'
).to
expect(mockInputBox).not.to.have.been.called
expect(cwd, 'no cwd is returned').to.be.undefined
})

it('should return a cwd if there is an invalid pipeline (let command fail)', async () => {
const { pipeline } = buildPipeline({
disposer: disposable,
Expand Down
8 changes: 8 additions & 0 deletions extension/src/test/suite/pipeline/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { Pipeline } from '../../../pipeline'
import { PipelineData } from '../../../pipeline/data'
import { dvcDemoPath } from '../../util'
import { buildDependencies } from '../util'
import { Response } from '../../../vscode/response'
import { Modal } from '../../../vscode/modal'

export const buildExperimentsPipeline = ({
disposer,
Expand Down Expand Up @@ -56,9 +58,15 @@ export const buildPipeline = ({
dvcYamls,
internalCommands
})

const mockShowInformation = stub(Modal, 'showInformation').resolves(
Response.YES
)

return {
dvcReader,
internalCommands,
mockShowInformation,
pipeline,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
pipelineData: (pipeline as any).data as PipelineData
Expand Down

0 comments on commit a5a40a9

Please sign in to comment.