Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
104 changes: 104 additions & 0 deletions .github/workflows/filecopymethod-test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
# Some platform-specific file copy syscalls (e.g. creating reflinks) are only
# supported on some platforms, and only with specific filesystems. These
# syscalls are used by different FileCopyMethod implementations.
#
# This workflow sets up the conditions needed for those syscalls to work,
# and then runs the tests with the different FileCopyMethods.

name: FileCopyMethod

on:
push:
branches: [ main, develop ]
pull_request:
branches: [ main, develop ]
workflow_dispatch:

jobs:
test-linux:
name: Test Linux
runs-on: ubuntu-latest
strategy:
matrix:
environment:
- filesystem: btrfs
filecopymethod: CopyBytes
steps:
- name: Set up Go
uses: actions/setup-go@v4
with:
go-version: 'stable'

- name: Check out code into the Go module directory
uses: actions/checkout@v3

- name: Get dependencies
run: go get -v -t -d ./...

- name: Build
run: go build -v .

- name: Set up filesystem
run: |-
mkdir -p ./test/filesystem
IMAGE_PATH="./test/filesystem/contents.img"
MOUNT_PATH="./test/filesystem/mounted"

truncate -s 500m "$IMAGE_PATH"
mkfs -t "${{matrix.environment.filesystem}}" "$IMAGE_PATH"
mkdir "$MOUNT_PATH"
sudo mount -o loop "$IMAGE_PATH" "$MOUNT_PATH"
sudo chown -R "$(id -u):$(id -g)" "$MOUNT_PATH"

- name: Copy files to mounted filesystem
run: |-
rsync -av --exclude=".*" --exclude "test/filesystem" . "test/filesystem/mounted"

- name: Test
working-directory: test/filesystem/mounted
run: go test -v
env:
TEST_FILECOPYMETHOD: ${{matrix.environment.filecopymethod}}


test-macos:
name: Test MacOS
runs-on: macos-latest
strategy:
matrix:
environment:
- filesystem: APFS
filecopymethod: CopyBytes
steps:
- name: Set up Go
uses: actions/setup-go@v4
with:
go-version: 'stable'

- name: Check out code into the Go module directory
uses: actions/checkout@v3

- name: Get dependencies
run: go get -v -t -d ./...

- name: Build
run: go build -v .

- name: Set up filesystem (MacOS)
run: |-
mkdir -p ./test/filesystem
IMAGE_PATH="./test/filesystem/contents.dmg"
MOUNT_PATH="./test/filesystem/mounted"

hdiutil create -size 500m -fs "${{matrix.environment.filesystem}}" "$IMAGE_PATH"
hdiutil attach -mountpoint "$MOUNT_PATH" "$IMAGE_PATH"

- name: Copy files to mounted filesystem
run: |-
rsync -av --exclude=".*" --exclude "test/filesystem" . "test/filesystem/mounted"

- name: Test
working-directory: test/filesystem/mounted
run: go test -v
env:
TEST_FILECOPYMETHOD: ${{matrix.environment.filecopymethod}}
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
test/data.copy
test/filesystem
test/owned-by-root
coverage.txt
vendor
Expand Down
36 changes: 32 additions & 4 deletions all_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,27 @@ import (
//go:embed test/data/case18/assets
var assets embed.FS

var currentFileCopyMethod FileCopyMethod
Copy link
Owner

Choose a reason for hiding this comment

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

We don't need this.


func setupFileCopyMethod(m *testing.M) {
Copy link
Owner

Choose a reason for hiding this comment

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

We don't need this func.

switch os.Getenv("TEST_FILECOPYMETHOD") {
case "":
currentFileCopyMethod = getDefaultOptions("", "").FileCopyMethod // Should be CopyBytes
case "CopyBytes":
currentFileCopyMethod = CopyBytes
}

// Allow running all the tests with a different FileCopyMethod.
// We want to re-use tests designed for CopyBytes with other copy methods
// to make sure that they behave the same way (where possible).
overrideDefaultOptions_FOR_TESTS = func(defopt *Options) {
defopt.FileCopyMethod = currentFileCopyMethod
}
}

func TestMain(m *testing.M) {
setup(m)
setupFileCopyMethod(m)
code := m.Run()
teardown(m)
os.Exit(code)
Expand Down Expand Up @@ -351,7 +370,6 @@ func TestOptions_PreserveOwner(t *testing.T) {
}

func TestOptions_CopyRateLimit(t *testing.T) {

file, err := os.Create("test/data/case16/large.file")
if err != nil {
t.Errorf("failed to create test file: %v", err)
Expand All @@ -372,8 +390,13 @@ func TestOptions_CopyRateLimit(t *testing.T) {
start := time.Now()
err = Copy("test/data/case16", "test/data.copy/case16", opt)
elapsed := time.Since(start)
Expect(t, err).ToBe(nil)
Expect(t, elapsed > 5*time.Second).ToBe(true)
if currentFileCopyMethod.supportsOptWrapReader {
Copy link
Owner

Choose a reason for hiding this comment

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

Regarding WrapReader only works with FileCopyMethod = CopyBytes, we can explicitly set

Options{
    WrapReader: func(src io.Reader) io.Reader { ... },
    FileCopyMethod: CopyBytes,
}

Thus, we don't need currentFileCopyMethod.supportsOptWrapReader

Expect(t, err).ToBe(nil)
Expect(t, elapsed > 5*time.Second).ToBe(true)
} else {
Expect(t, err).Not().ToBe(nil)
Expect(t, errors.Is(err, ErrUnsupportedCopyMethod)).ToBe(true)
}
}

func TestOptions_OnFileError(t *testing.T) {
Expand Down Expand Up @@ -422,7 +445,12 @@ func TestOptions_FS(t *testing.T) {
FS: assets,
PermissionControl: AddPermission(200), // FIXME
})
Expect(t, err).ToBe(nil)
if currentFileCopyMethod.supportsOptFS {
Copy link
Owner

Choose a reason for hiding this comment

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

Same

Expect(t, err).ToBe(nil)
} else {
Expect(t, err).Not().ToBe(nil)
Expect(t, errors.Is(err, ErrUnsupportedCopyMethod)).ToBe(true)
}
}

type SleepyReader struct {
Expand Down
2 changes: 2 additions & 0 deletions copy_methods.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ var ErrUnsupportedCopyMethod = errors.New(
// CopyBytes copies the file contents by reading the source file into a buffer,
// then writing the buffer back to the destination file.
var CopyBytes = FileCopyMethod{
supportsOptFS: true,
supportsOptWrapReader: true,
Copy link
Owner

Choose a reason for hiding this comment

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

as mentioned above, we don't need these

fcopy: func(src, dest string, info os.FileInfo, opt Options) (err error, skipFile bool) {
var readcloser io.ReadCloser
if opt.FS != nil {
Expand Down
16 changes: 15 additions & 1 deletion options.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,12 +132,15 @@ const (
// FileCopyMethod represents one of the ways that a regular file can be copied.
type FileCopyMethod struct {
fcopy func(src, dest string, info os.FileInfo, opt Options) (err error, skipFile bool)

supportsOptFS bool
supportsOptWrapReader bool
}

// getDefaultOptions provides default options,
// which would be modified by usage-side.
func getDefaultOptions(src, dest string) Options {
return Options{
defopt := Options{
OnSymlink: func(string) SymlinkAction {
return Shallow // Do shallow copy
},
Expand All @@ -154,8 +157,19 @@ func getDefaultOptions(src, dest string) Options {
WrapReader: nil, // Do not wrap src files, use them as they are.
intent: intent{src, dest, nil, nil},
}

if overrideDefaultOptions_FOR_TESTS != nil {
overrideDefaultOptions_FOR_TESTS(&defopt)
}

return defopt
}

// overrideDefaultOptions_FOR_TESTS allows the copy package tests to replace
// the default options. This allows existing test code to be reused with
// different settings as a way to check that behavior is consistent.
var overrideDefaultOptions_FOR_TESTS func(*Options)
Copy link
Owner

Choose a reason for hiding this comment

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

Please do not include any test-specific logic inside main package.

Copy link
Contributor Author

@eth-p eth-p Sep 30, 2024

Choose a reason for hiding this comment

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

As mentioned in my other comments: there is no perfect way to use the existing tests with any other FileCopyMethod than the default. Something has to be made changed for this to be possible:

What we tried before

Wrapping the Copy function inside tests

With #160, I made a CopyInTest function for test code that adds an option when no options are provided or when the option is missing from the Options struct argument. This idea was rejected:

We shouldn't use CopyInTest as it doesn't represents users use-cases. We should avoid any special logic in the tests as much as possible.

Creating a defaultCopyMethod that only changes inside tests

That was the last version of this pull request. The idea was also rejected:

Package should be testable.
It doesn't mean package should embrace some noise for testing.

Please do not include source code which is needed for testing specifically.

and

We don't need it.
Users can set their opt.FileCopyMethod by theirselves, NOT by env or any other implicit mechanism.

and

Same here. We should take a position: default FileCopyMethod is CopyBytes, nothing else. Opt is opt, where users can control, not that environment can control.

and

Overall, let's use CopyBytes always in all platform by default.

I prefer simpleness and customizability, over implicit kindness.

Maybe there was a misunderstanding or miscommunication. When I tried that way, the user can not set the default. That "default" is internal to copy and only changes in the CI to use in the filecopymethod-test.yml workflow.

What we could do

Have a global variable in all_test.go

Change all the tests to set the FileCopyMethod in options using a global variable defined in all_test.go:

  func TestOptions_PreserveOwner(t *testing.T) {
- 	opt := Options{PreserveOwner: true}
+ 	opt := Options{PreserveOwner: true, FileCopyMethod: testingFileCopyMethod}
  	err := Copy("test/data/case13", "test/data.copy/case13", opt)
  	Expect(t, err).ToBe(nil)
  }

This approach is harder to maintain than the other two:

  • New tests have to always include the FileCopyMethod, and that is easy to forget.
  • Noisy.
    • DRY.
    • Options always have to be added to Copy()
    • More complicated example for the user than it needs to be.

Copy the code of tests

If we we can never change the implicit FileCopyMethod , we would have to duplicate test code. The PreserveTimes, PreserveOwner, PermissionControl, Sync, FS, WrapReader options rely on the FileCopyMethod implementation, as well as these test When() cases:

  • file is deleted while copying
  • symlink is deleted while copying
  • try to copy a directory that has no write permission and copy file inside along with it
  • try to copy READ-not-allowed source
  • try to copy a file to existing path

Instead of just having only the existing tests, we would have to make new tests to check all those for every different FileCopyMethod. In my opinion, that is approach is bad with many problems:

  • DRY
  • Bad at catching regressions.
    • Only some behaviour is checked with different FileCopyMethods
    • New tests do not check other FileCopyMethod than default
  • Not maintainable.
    • Changes need to be made to both copies of the tests.

Only making new tests

We could choose to not re-use existing tests and only add new ones, but that is a very bad idea for the reason I said above:

  • Bad at catching regressions.
    • Only some behaviour is checked with different FileCopyMethods
    • New tests do not check other FileCopyMethod than default

Only making new tests

We could choose to not re-use existing tests and only add new ones, but that is a very bad idea for the reason I said above:

  • Bad at catching regressions.
    • Only some behaviour is checked with different FileCopyMethods
    • New tests do not check other FileCopyMethod than default

Go build tags

We could use a build tag to set the default FileCopyMethod for running in the CI matrix tests, but:

  • Build tags are per-project, not per-package.
    • This lets the user set the default with a build tag.
    • The default should always be CopyBytes.

Please tell me which option you prefer, and I'll go with that one.

I liked "creating a defaultCopyMethod that only changes inside tests" because it let the CI workflow test everything with the different FileCopyMethod (matrix test), and did not make the code much more complex or put maintainability burden on the developer.


// assureOptions struct, should be called only once.
// All optional values MUST NOT BE nil/zero after assured.
func assureOptions(src, dest string, opts ...Options) Options {
Expand Down