Skip to content

Commit 0e6c5be

Browse files
julieg18julieg18
authored
Fix Toast messages showing a successful message after failed remote command (#5105)
1 parent 197d438 commit 0e6c5be

File tree

4 files changed

+167
-76
lines changed

4 files changed

+167
-76
lines changed

‎extension/src/cli/dvc/config.test.ts‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,5 +104,95 @@ describe('DvcConfig', () => {
104104
executable: 'dvc'
105105
})
106106
})
107+
108+
it('should return undefined if the underlying process throws', async () => {
109+
const cwd = __dirname
110+
111+
mockedCreateProcess.mockImplementationOnce(() => {
112+
throw new Error('remote does not exist')
113+
})
114+
115+
const output = await dvcConfig.remote(
116+
cwd,
117+
SubCommand.REMOVE,
118+
Flag.PROJECT,
119+
'remote-name'
120+
)
121+
expect(output).toStrictEqual(undefined)
122+
})
123+
})
124+
125+
describe('remoteAdd', () => {
126+
it('should call createProcess with the correct parameters to add a remote to the config', async () => {
127+
const cwd = __dirname
128+
const stdout = ''
129+
130+
mockedCreateProcess.mockReturnValueOnce(getMockedProcess(stdout))
131+
132+
const storage = 'storage'
133+
const url = 'url.com'
134+
const output = await dvcConfig.remoteAdd(cwd, Flag.PROJECT, storage, url)
135+
expect(output).toStrictEqual(stdout)
136+
137+
expect(mockedCreateProcess).toHaveBeenCalledWith({
138+
args: ['remote', 'add', '--project', storage, url],
139+
cwd,
140+
env: mockedEnv,
141+
executable: 'dvc'
142+
})
143+
})
144+
})
145+
146+
describe('remoteRename', () => {
147+
it('should call createProcess with the correct parameters to rename a remote name in the config', async () => {
148+
const cwd = __dirname
149+
const stdout = ''
150+
151+
mockedCreateProcess.mockReturnValueOnce(getMockedProcess(stdout))
152+
153+
const oldName = 'storagge'
154+
const newName = 'storage'
155+
const output = await dvcConfig.remoteRename(
156+
cwd,
157+
Flag.LOCAL,
158+
oldName,
159+
newName
160+
)
161+
expect(output).toStrictEqual(stdout)
162+
163+
expect(mockedCreateProcess).toHaveBeenCalledWith({
164+
args: ['remote', 'rename', '--local', oldName, newName],
165+
cwd,
166+
env: mockedEnv,
167+
executable: 'dvc'
168+
})
169+
})
170+
})
171+
172+
describe('remoteModify', () => {
173+
it('should call createProcess with the correct parameters to modify a remote url in the config', async () => {
174+
const cwd = __dirname
175+
const stdout = ''
176+
177+
mockedCreateProcess.mockReturnValueOnce(getMockedProcess(stdout))
178+
179+
const name = 'storage'
180+
const newUrl = 'url.com'
181+
const output = await dvcConfig.remoteModify(
182+
cwd,
183+
Flag.PROJECT,
184+
name,
185+
'url',
186+
newUrl
187+
)
188+
expect(output).toStrictEqual(stdout)
189+
190+
expect(mockedCreateProcess).toHaveBeenCalledWith({
191+
args: ['remote', 'modify', '--project', name, 'url', newUrl],
192+
cwd,
193+
env: mockedEnv,
194+
executable: 'dvc'
195+
})
196+
})
107197
})
108198
})

‎extension/src/cli/dvc/config.ts‎

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
import { DvcCli } from '.'
2-
import { Args, Command } from './constants'
2+
import { Args, Command, SubCommand } from './constants'
33
import { typeCheckCommands } from '..'
44

55
export const autoRegisteredCommands = {
66
CONFIG: 'config',
7-
REMOTE: 'remote'
7+
REMOTE: 'remote',
8+
REMOTE_ADD: 'remoteAdd',
9+
REMOTE_MODIFY: 'remoteModify',
10+
REMOTE_RENAME: 'remoteRename'
811
} as const
912

1013
export class DvcConfig extends DvcCli {
@@ -21,6 +24,28 @@ export class DvcConfig extends DvcCli {
2124
return this.executeSafeProcess(cwd, Command.REMOTE, ...args)
2225
}
2326

27+
public remoteAdd(cwd: string, ...args: Args) {
28+
return this.executeDvcProcess(cwd, Command.REMOTE, SubCommand.ADD, ...args)
29+
}
30+
31+
public remoteRename(cwd: string, ...args: Args) {
32+
return this.executeDvcProcess(
33+
cwd,
34+
Command.REMOTE,
35+
SubCommand.RENAME,
36+
...args
37+
)
38+
}
39+
40+
public remoteModify(cwd: string, ...args: Args) {
41+
return this.executeDvcProcess(
42+
cwd,
43+
Command.REMOTE,
44+
SubCommand.MODIFY,
45+
...args
46+
)
47+
}
48+
2449
private async executeSafeProcess(
2550
cwd: string,
2651
command: Command,

‎extension/src/setup/commands/index.ts‎

Lines changed: 17 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -91,9 +91,8 @@ export const addRemoteToProject = async (
9191

9292
return await Toast.showOutput(
9393
internalCommands.executeCommand(
94-
AvailableCommands.REMOTE,
94+
AvailableCommands.REMOTE_ADD,
9595
dvcRoot,
96-
SubCommand.ADD,
9796
...args
9897
)
9998
)
@@ -135,9 +134,8 @@ const modifyRemoteName = async (
135134
}
136135
return await Toast.showOutput(
137136
internalCommands.executeCommand(
138-
AvailableCommands.REMOTE,
137+
AvailableCommands.REMOTE_RENAME,
139138
dvcRoot,
140-
SubCommand.RENAME,
141139
config,
142140
name,
143141
newName
@@ -157,9 +155,8 @@ const modifyRemoteUrl = async (
157155
}
158156
return await Toast.showOutput(
159157
internalCommands.executeCommand(
160-
AvailableCommands.REMOTE,
158+
AvailableCommands.REMOTE_MODIFY,
161159
dvcRoot,
162-
SubCommand.MODIFY,
163160
config,
164161
name,
165162
'url',
@@ -275,25 +272,21 @@ const confirmAndRemove = async (
275272
return
276273
}
277274

278-
try {
279-
await internalCommands.executeCommand(
280-
AvailableCommands.REMOTE,
281-
dvcRoot,
282-
SubCommand.REMOVE,
283-
Flag.PROJECT,
284-
remote
285-
)
286-
} catch {}
275+
await internalCommands.executeCommand(
276+
AvailableCommands.REMOTE,
277+
dvcRoot,
278+
SubCommand.REMOVE,
279+
Flag.PROJECT,
280+
remote
281+
)
287282

288-
try {
289-
await internalCommands.executeCommand(
290-
AvailableCommands.REMOTE,
291-
dvcRoot,
292-
SubCommand.REMOVE,
293-
Flag.LOCAL,
294-
remote
295-
)
296-
} catch {}
283+
await internalCommands.executeCommand(
284+
AvailableCommands.REMOTE,
285+
dvcRoot,
286+
SubCommand.REMOVE,
287+
Flag.LOCAL,
288+
remote
289+
)
297290
}
298291

299292
export const pickRemoteAndRemove = async (

‎extension/src/test/suite/setup/index.test.ts‎

Lines changed: 33 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1465,13 +1465,12 @@ suite('Setup Test Suite', () => {
14651465

14661466
const mockMessageReceived = getMessageReceivedEmitter(webview)
14671467

1468-
const mockRemote = stub(DvcConfig.prototype, 'remote')
1468+
stub(DvcConfig.prototype, 'remote').resolves('')
1469+
const mockRemoteAdd = stub(DvcConfig.prototype, 'remoteAdd')
14691470

14701471
const remoteAdded = new Promise(resolve =>
1471-
mockRemote.callsFake((_, ...args) => {
1472-
if (args.includes('add')) {
1473-
resolve(undefined)
1474-
}
1472+
mockRemoteAdd.callsFake(() => {
1473+
resolve(undefined)
14751474
return Promise.resolve('')
14761475
})
14771476
)
@@ -1491,11 +1490,10 @@ suite('Setup Test Suite', () => {
14911490

14921491
expect(mockShowInputBox).to.be.calledTwice
14931492
expect(
1494-
mockRemote,
1493+
mockRemoteAdd,
14951494
'new remote is set as the default'
14961495
).to.be.calledWithExactly(
14971496
dvcDemoPath,
1498-
'add',
14991497
'-d',
15001498
'--project',
15011499
'storage',
@@ -1504,17 +1502,12 @@ suite('Setup Test Suite', () => {
15041502
}).timeout(WEBVIEW_TEST_TIMEOUT)
15051503

15061504
it('should be able to add a remote', async () => {
1507-
const mockRemote = stub(DvcConfig.prototype, 'remote')
1505+
stub(DvcConfig.prototype, 'remote').resolves('storage s3://my-bucket')
1506+
const mockRemoteAdd = stub(DvcConfig.prototype, 'remoteAdd')
15081507

15091508
const remoteAdded = new Promise(resolve =>
1510-
mockRemote.callsFake((_, ...args) => {
1511-
if (args.includes('list')) {
1512-
return Promise.resolve('storage s3://my-bucket')
1513-
}
1514-
1515-
if (args.includes('add')) {
1516-
resolve(undefined)
1517-
}
1509+
mockRemoteAdd.callsFake(() => {
1510+
resolve(undefined)
15181511
return Promise.resolve('')
15191512
})
15201513
)
@@ -1542,34 +1535,29 @@ suite('Setup Test Suite', () => {
15421535
expect(mockShowInputBox).to.be.calledTwice
15431536
expect(mockShowQuickPick).to.be.calledOnce
15441537
expect(
1545-
mockRemote,
1538+
mockRemoteAdd,
15461539
'should not set a remote as the default unless the user explicitly chooses to'
15471540
).to.be.calledWithExactly(
15481541
dvcDemoPath,
1549-
'add',
15501542
'--project',
15511543
'backup',
15521544
's3://my-backup-bucket'
15531545
)
15541546
}).timeout(WEBVIEW_TEST_TIMEOUT)
15551547

15561548
it('should be able to rename a remote', async () => {
1557-
const mockRemote = stub(DvcConfig.prototype, 'remote')
1549+
stub(DvcConfig.prototype, 'remote').callsFake((_, ...args) => {
1550+
if (args.includes('list') && args.includes('--project')) {
1551+
return Promise.resolve('storage s3://my-bucket')
1552+
}
1553+
return Promise.resolve('')
1554+
})
1555+
const mockRemoteRename = stub(DvcConfig.prototype, 'remoteRename')
15581556
const newName = 'better-name'
15591557

15601558
const remoteRenamed = new Promise(resolve =>
1561-
mockRemote.callsFake((_, ...args) => {
1562-
if (args.includes('list') && args.includes('--project')) {
1563-
return Promise.resolve('storage s3://my-bucket')
1564-
}
1565-
1566-
if (args.includes('list')) {
1567-
return Promise.resolve('')
1568-
}
1569-
1570-
if (args.includes('rename')) {
1571-
resolve(undefined)
1572-
}
1559+
mockRemoteRename.callsFake(() => {
1560+
resolve(undefined)
15731561
return Promise.resolve('')
15741562
})
15751563
)
@@ -1589,9 +1577,8 @@ suite('Setup Test Suite', () => {
15891577

15901578
expect(mockShowInputBox).to.be.calledOnce
15911579
expect(mockShowQuickPick).to.be.calledOnce
1592-
expect(mockRemote).to.be.calledWithExactly(
1580+
expect(mockRemoteRename).to.be.calledWithExactly(
15931581
dvcDemoPath,
1594-
'rename',
15951582
'--project',
15961583
'storage',
15971584
newName
@@ -1609,24 +1596,21 @@ suite('Setup Test Suite', () => {
16091596

16101597
const mockMessageReceived = getMessageReceivedEmitter(webview)
16111598

1612-
const mockRemote = stub(DvcConfig.prototype, 'remote')
16131599
const projectConfigUrl = 's3://different-url'
1600+
stub(DvcConfig.prototype, 'remote').callsFake((_, ...args) => {
1601+
if (args.includes('list') && args.includes('--project')) {
1602+
return Promise.resolve(
1603+
`storage ${projectConfigUrl}\nbackup s3://my-backup-bucket`
1604+
)
1605+
}
16141606

1615-
const remoteModified = new Promise(resolve =>
1616-
mockRemote.callsFake((_, ...args) => {
1617-
if (args.includes('list') && args.includes('--project')) {
1618-
return Promise.resolve(
1619-
`storage ${projectConfigUrl}\nbackup s3://my-backup-bucket`
1620-
)
1621-
}
1622-
1623-
if (args.includes('list') && args.includes('--local')) {
1624-
return Promise.resolve('storage s3://my-bucket')
1625-
}
1607+
return Promise.resolve('storage s3://my-bucket')
1608+
})
1609+
const mockRemoteRename = stub(DvcConfig.prototype, 'remoteModify')
16261610

1627-
if (args.includes('modify')) {
1628-
resolve(undefined)
1629-
}
1611+
const remoteModified = new Promise(resolve =>
1612+
mockRemoteRename.callsFake(() => {
1613+
resolve(undefined)
16301614
return Promise.resolve('')
16311615
})
16321616
)
@@ -1662,9 +1646,8 @@ suite('Setup Test Suite', () => {
16621646

16631647
expect(mockShowInputBox).to.be.calledOnce
16641648
expect(mockShowQuickPick).to.be.calledTwice
1665-
expect(mockRemote).to.be.calledWithExactly(
1649+
expect(mockRemoteRename).to.be.calledWithExactly(
16661650
dvcDemoPath,
1667-
'modify',
16681651
'--local',
16691652
'storage',
16701653
'url',

0 commit comments

Comments
 (0)