Skip to content

Commit dcc67e6

Browse files
authored
Update integration tests for the fs command (#3091)
## Changes * Fix potential panic * Reuse ctx variable ## Why I observed the following panic. The code could access `r` after a failed assertion here: https://github.com/databricks/cli/blob/fb2b646997789ef15d89c805f2b77548e4665dfe/integration/cmd/fs/cp_test.go#L41-L46 ``` === FAIL: integration/cmd/fs TestFsCpFileToFile (99.64s) panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x11e4b13] goroutine 344 [running]: testing.tRunner.func1.2({0x1450540, 0x25334c0}) /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.linux-amd64/src/testing/testing.go:1734 +0x21c testing.tRunner.func1() /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.linux-amd64/src/testing/testing.go:1737 +0x35e panic({0x1450540?, 0x25334c0?}) /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.linux-amd64/src/runtime/panic.go:792 +0x132 github.com/databricks/cli/integration/cmd/fs_test.assertTargetFile(0xc0004b4540, {0x19b5c60?, 0x2589080?}, {0x19bc0b0?, 0xc00111c980?}, {0x1734a1a?, 0xc0004b4540?}) /home/runner/work/eng-dev-ecosystem/eng-dev-ecosystem/ext/cli/integration/cmd/fs/cp_test.go:43 +0x93 github.com/databricks/cli/integration/cmd/fs_test.TestFsCpFileToFile.func1(0xc0004b4540) /home/runner/work/eng-dev-ecosystem/eng-dev-ecosystem/ext/cli/integration/cmd/fs/cp_test.go:158 +0x2a5 testing.tRunner(0xc0004b4540, 0xc000551170) /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.linux-amd64/src/testing/testing.go:1792 +0xf4 created by testing.(*T).Run in goroutine 96 /home/runner/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.24.2.linux-amd64/src/testing/testing.go:1851 +0x413 ``` ## Tests Tests still pass.
1 parent fb2b646 commit dcc67e6

File tree

1 file changed

+40
-39
lines changed

1 file changed

+40
-39
lines changed

integration/cmd/fs/cp_test.go

Lines changed: 40 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -36,22 +36,24 @@ func setupSourceFile(t *testing.T, ctx context.Context, f filer.Filer) {
3636
}
3737

3838
func assertTargetFile(t *testing.T, ctx context.Context, f filer.Filer, relPath string) {
39-
var err error
40-
41-
r, err := f.Read(ctx, relPath)
42-
assert.NoError(t, err)
43-
defer r.Close()
44-
b, err := io.ReadAll(r)
45-
require.NoError(t, err)
46-
assert.Equal(t, "abc", string(b))
39+
assertFileContent(t, ctx, f, relPath, "abc")
4740
}
4841

4942
func assertFileContent(t *testing.T, ctx context.Context, f filer.Filer, path, expectedContent string) {
5043
r, err := f.Read(ctx, path)
51-
require.NoError(t, err)
44+
if err != nil {
45+
assert.NoError(t, err)
46+
return
47+
}
48+
5249
defer r.Close()
50+
5351
b, err := io.ReadAll(r)
54-
require.NoError(t, err)
52+
if err != nil {
53+
assert.NoError(t, err)
54+
return
55+
}
56+
5557
assert.Equal(t, expectedContent, string(b))
5658
}
5759

@@ -132,11 +134,11 @@ func TestFsCpDir(t *testing.T) {
132134
ctx := context.Background()
133135
sourceFiler, sourceDir := testCase.setupSource(t)
134136
targetFiler, targetDir := testCase.setupTarget(t)
135-
setupSourceDir(t, context.Background(), sourceFiler)
137+
setupSourceDir(t, ctx, sourceFiler)
136138

137139
testcli.RequireSuccessfulRun(t, ctx, "fs", "cp", sourceDir, targetDir, "--recursive")
138140

139-
assertTargetDir(t, context.Background(), targetFiler)
141+
assertTargetDir(t, ctx, targetFiler)
140142
})
141143
}
142144
}
@@ -151,11 +153,10 @@ func TestFsCpFileToFile(t *testing.T) {
151153
ctx := context.Background()
152154
sourceFiler, sourceDir := testCase.setupSource(t)
153155
targetFiler, targetDir := testCase.setupTarget(t)
154-
setupSourceFile(t, context.Background(), sourceFiler)
156+
setupSourceFile(t, ctx, sourceFiler)
155157

156158
testcli.RequireSuccessfulRun(t, ctx, "fs", "cp", path.Join(sourceDir, "foo.txt"), path.Join(targetDir, "bar.txt"))
157-
158-
assertTargetFile(t, context.Background(), targetFiler, "bar.txt")
159+
assertTargetFile(t, ctx, targetFiler, "bar.txt")
159160
})
160161
}
161162
}
@@ -170,11 +171,11 @@ func TestFsCpFileToDir(t *testing.T) {
170171
ctx := context.Background()
171172
sourceFiler, sourceDir := testCase.setupSource(t)
172173
targetFiler, targetDir := testCase.setupTarget(t)
173-
setupSourceFile(t, context.Background(), sourceFiler)
174+
setupSourceFile(t, ctx, sourceFiler)
174175

175176
testcli.RequireSuccessfulRun(t, ctx, "fs", "cp", path.Join(sourceDir, "foo.txt"), targetDir)
176177

177-
assertTargetFile(t, context.Background(), targetFiler, "foo.txt")
178+
assertTargetFile(t, ctx, targetFiler, "foo.txt")
178179
})
179180
}
180181
}
@@ -205,16 +206,16 @@ func TestFsCpDirToDirFileNotOverwritten(t *testing.T) {
205206
ctx := context.Background()
206207
sourceFiler, sourceDir := testCase.setupSource(t)
207208
targetFiler, targetDir := testCase.setupTarget(t)
208-
setupSourceDir(t, context.Background(), sourceFiler)
209+
setupSourceDir(t, ctx, sourceFiler)
209210

210211
// Write a conflicting file to target
211-
err := targetFiler.Write(context.Background(), "a/b/c/hello.txt", strings.NewReader("this should not be overwritten"), filer.CreateParentDirectories)
212+
err := targetFiler.Write(ctx, "a/b/c/hello.txt", strings.NewReader("this should not be overwritten"), filer.CreateParentDirectories)
212213
require.NoError(t, err)
213214

214215
testcli.RequireSuccessfulRun(t, ctx, "fs", "cp", sourceDir, targetDir, "--recursive")
215-
assertFileContent(t, context.Background(), targetFiler, "a/b/c/hello.txt", "this should not be overwritten")
216-
assertFileContent(t, context.Background(), targetFiler, "query.sql", "SELECT 1")
217-
assertFileContent(t, context.Background(), targetFiler, "pyNb.py", "# Databricks notebook source\nprint(123)")
216+
assertFileContent(t, ctx, targetFiler, "a/b/c/hello.txt", "this should not be overwritten")
217+
assertFileContent(t, ctx, targetFiler, "query.sql", "SELECT 1")
218+
assertFileContent(t, ctx, targetFiler, "pyNb.py", "# Databricks notebook source\nprint(123)")
218219
})
219220
}
220221
}
@@ -229,14 +230,14 @@ func TestFsCpFileToDirFileNotOverwritten(t *testing.T) {
229230
ctx := context.Background()
230231
sourceFiler, sourceDir := testCase.setupSource(t)
231232
targetFiler, targetDir := testCase.setupTarget(t)
232-
setupSourceDir(t, context.Background(), sourceFiler)
233+
setupSourceDir(t, ctx, sourceFiler)
233234

234235
// Write a conflicting file to target
235-
err := targetFiler.Write(context.Background(), "a/b/c/hello.txt", strings.NewReader("this should not be overwritten"), filer.CreateParentDirectories)
236+
err := targetFiler.Write(ctx, "a/b/c/hello.txt", strings.NewReader("this should not be overwritten"), filer.CreateParentDirectories)
236237
require.NoError(t, err)
237238

238239
testcli.RequireSuccessfulRun(t, ctx, "fs", "cp", path.Join(sourceDir, "a/b/c/hello.txt"), path.Join(targetDir, "a/b/c"))
239-
assertFileContent(t, context.Background(), targetFiler, "a/b/c/hello.txt", "this should not be overwritten")
240+
assertFileContent(t, ctx, targetFiler, "a/b/c/hello.txt", "this should not be overwritten")
240241
})
241242
}
242243
}
@@ -251,14 +252,14 @@ func TestFsCpFileToFileFileNotOverwritten(t *testing.T) {
251252
ctx := context.Background()
252253
sourceFiler, sourceDir := testCase.setupSource(t)
253254
targetFiler, targetDir := testCase.setupTarget(t)
254-
setupSourceDir(t, context.Background(), sourceFiler)
255+
setupSourceDir(t, ctx, sourceFiler)
255256

256257
// Write a conflicting file to target
257-
err := targetFiler.Write(context.Background(), "a/b/c/dontoverwrite.txt", strings.NewReader("this should not be overwritten"), filer.CreateParentDirectories)
258+
err := targetFiler.Write(ctx, "a/b/c/dontoverwrite.txt", strings.NewReader("this should not be overwritten"), filer.CreateParentDirectories)
258259
require.NoError(t, err)
259260

260261
testcli.RequireSuccessfulRun(t, ctx, "fs", "cp", path.Join(sourceDir, "a/b/c/hello.txt"), path.Join(targetDir, "a/b/c/dontoverwrite.txt"))
261-
assertFileContent(t, context.Background(), targetFiler, "a/b/c/dontoverwrite.txt", "this should not be overwritten")
262+
assertFileContent(t, ctx, targetFiler, "a/b/c/dontoverwrite.txt", "this should not be overwritten")
262263
})
263264
}
264265
}
@@ -273,14 +274,14 @@ func TestFsCpDirToDirWithOverwriteFlag(t *testing.T) {
273274
ctx := context.Background()
274275
sourceFiler, sourceDir := testCase.setupSource(t)
275276
targetFiler, targetDir := testCase.setupTarget(t)
276-
setupSourceDir(t, context.Background(), sourceFiler)
277+
setupSourceDir(t, ctx, sourceFiler)
277278

278279
// Write a conflicting file to target
279-
err := targetFiler.Write(context.Background(), "a/b/c/hello.txt", strings.NewReader("this should be overwritten"), filer.CreateParentDirectories)
280+
err := targetFiler.Write(ctx, "a/b/c/hello.txt", strings.NewReader("this should be overwritten"), filer.CreateParentDirectories)
280281
require.NoError(t, err)
281282

282283
testcli.RequireSuccessfulRun(t, ctx, "fs", "cp", sourceDir, targetDir, "--recursive", "--overwrite")
283-
assertTargetDir(t, context.Background(), targetFiler)
284+
assertTargetDir(t, ctx, targetFiler)
284285
})
285286
}
286287
}
@@ -295,14 +296,14 @@ func TestFsCpFileToFileWithOverwriteFlag(t *testing.T) {
295296
ctx := context.Background()
296297
sourceFiler, sourceDir := testCase.setupSource(t)
297298
targetFiler, targetDir := testCase.setupTarget(t)
298-
setupSourceDir(t, context.Background(), sourceFiler)
299+
setupSourceDir(t, ctx, sourceFiler)
299300

300301
// Write a conflicting file to target
301-
err := targetFiler.Write(context.Background(), "a/b/c/overwritten.txt", strings.NewReader("this should be overwritten"), filer.CreateParentDirectories)
302+
err := targetFiler.Write(ctx, "a/b/c/overwritten.txt", strings.NewReader("this should be overwritten"), filer.CreateParentDirectories)
302303
require.NoError(t, err)
303304

304305
testcli.RequireSuccessfulRun(t, ctx, "fs", "cp", path.Join(sourceDir, "a/b/c/hello.txt"), path.Join(targetDir, "a/b/c/overwritten.txt"), "--overwrite")
305-
assertFileContent(t, context.Background(), targetFiler, "a/b/c/overwritten.txt", "hello, world\n")
306+
assertFileContent(t, ctx, targetFiler, "a/b/c/overwritten.txt", "hello, world\n")
306307
})
307308
}
308309
}
@@ -317,14 +318,14 @@ func TestFsCpFileToDirWithOverwriteFlag(t *testing.T) {
317318
ctx := context.Background()
318319
sourceFiler, sourceDir := testCase.setupSource(t)
319320
targetFiler, targetDir := testCase.setupTarget(t)
320-
setupSourceDir(t, context.Background(), sourceFiler)
321+
setupSourceDir(t, ctx, sourceFiler)
321322

322323
// Write a conflicting file to target
323-
err := targetFiler.Write(context.Background(), "a/b/c/hello.txt", strings.NewReader("this should be overwritten"), filer.CreateParentDirectories)
324+
err := targetFiler.Write(ctx, "a/b/c/hello.txt", strings.NewReader("this should be overwritten"), filer.CreateParentDirectories)
324325
require.NoError(t, err)
325326

326327
testcli.RequireSuccessfulRun(t, ctx, "fs", "cp", path.Join(sourceDir, "a/b/c/hello.txt"), path.Join(targetDir, "a/b/c"), "--overwrite")
327-
assertFileContent(t, context.Background(), targetFiler, "a/b/c/hello.txt", "hello, world\n")
328+
assertFileContent(t, ctx, targetFiler, "a/b/c/hello.txt", "hello, world\n")
328329
})
329330
}
330331
}
@@ -362,10 +363,10 @@ func TestFsCpSourceIsDirectoryButTargetIsFile(t *testing.T) {
362363
ctx := context.Background()
363364
sourceFiler, sourceDir := testCase.setupSource(t)
364365
targetFiler, targetDir := testCase.setupTarget(t)
365-
setupSourceDir(t, context.Background(), sourceFiler)
366+
setupSourceDir(t, ctx, sourceFiler)
366367

367368
// Write a conflicting file to target
368-
err := targetFiler.Write(context.Background(), "my_target", strings.NewReader("I'll block any attempts to recursively copy"), filer.CreateParentDirectories)
369+
err := targetFiler.Write(ctx, "my_target", strings.NewReader("I'll block any attempts to recursively copy"), filer.CreateParentDirectories)
369370
require.NoError(t, err)
370371

371372
_, _, err = testcli.RequireErrorRun(t, ctx, "fs", "cp", sourceDir, path.Join(targetDir, "my_target"), "--recursive")

0 commit comments

Comments
 (0)