test: Add testing for FileCopyMethod #165

eth-p · 2024-08-25T01:17:08Z

~~This commit is stacked on top of #164. That must be reviewed and merged first.~~

Reason

To prevent regressions, we want good test coverage no matter the FileCopyMethod used. We also want to make sure other FileCopyMethods behave like CopyBytes when successful for consistency.

Instead of writing many tests to check the behaviour is the same, we re-use existing tests to check that other FileCopyMethods work like CopyBytes. The CI runs then runs these tests so the developer does not need different operating systems and the right filesystems installed.

Details

This series of commits changes the CI and testing so that:

The default FileCopyMethod can be changed just in tests using an environment variable.
The CI has more OS variants for using specific filesystems.
- e.g. btrfs for testing copy-on-write (aka reflink)
The CI can choose which FileCopyMethod to use.

Example

This was a run to ensure the copy-on-write (reflink) implementation works.

Previous runs helped me catch a couple edge cases:

opt.FS != nil
opt.WrapReader != nil

That would not have been as easy without these changes.

otiai10 · 2024-09-26T04:30:57Z

@eth-p Is this ready for review?

eth-p · 2024-09-26T15:43:39Z

Yep! Commits f530620 and older are from the last pull request. They will be removed once I do a rebase to main, so you can skip them if you want.

otiai10 · 2024-09-27T00:21:01Z

Could you rebase your branch first so that only the meaning full diff will show up?

eth-p · 2024-09-27T02:48:25Z

Could you rebase your branch first so that only the meaning full diff will show up?

Done

otiai10 · 2024-09-27T02:53:44Z

options.go

+// The default FileCopyMethod.
+// This only is changed during tests.
+var defaultCopyMethod = CopyBytes
+var defaultCopyMethodName = "CopyBytes"


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.

I want to use the existing tests to check that all FileCopyMethods behave the same way (where possible). There isn't any other reasonable way to do that, sadly. Other possible ways I considered were:

Add a new copy.SetDefaultFileCopyMethod(method FileCopyMethod) function.

Bad — Mutable global variables should be avoided.

Bad — Adds more exported functions to the package.

Copy all the tests again, but setting a different FileCopyMethod for each

Bad — DRY

Bad — Multiple copies = easier to forget to change all of them

Bad — Options always have to be added when calling copy.Copy

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

Good — No special code for testing.

Bad — Options always have to be added when calling copy.Copy

Bad — New tests have to be set up to include FileCopyMethod

Easy to forget to do

If happens, will break goal of running tests with all FileCopyMethods.

Option 3 is the closest, but I didn't feel that the risk of accidentally missing testing a FileCopyMethod was a good trade off to reduce 3 extra lines of test-specific code. What do you think?

Also, this part:

var defaultCopyMethodName = "CopyBytes"

Is only because Go does not allow comparing function pointers even when they are not a closure. (playground)

otiai10 · 2024-09-27T02:53:53Z

options.go

@@ -149,7 +154,7 @@ func getDefaultOptions(src, dest string) Options {
 		Sync:              false,              // Do not sync
 		Specials:          false,              // Do not copy special files
 		PreserveTimes:     false,              // Do not preserve the modification time
-		FileCopyMethod:    CopyBytes,          // Copy by bytes
+		FileCopyMethod:    defaultCopyMethod,  // Copy by bytes, unless testing this package


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.

otiai10 · 2024-09-27T03:00:53Z

.github/workflows/go.yml

@@ -32,7 +41,46 @@ jobs:
    - name: Build
      run: go build -v .

+    - name: Set up testing environment


Please separate yaml file and let's keep this basic test simple as it is.

Testing is not only to test our source code, but it is also a working example and source of confidence users can use this package on there environment.

When we describe so much steps to setup environment, this could be a message to users, such as, "You need to get this much preparation done to just get started"

Please separate yaml file and let's keep this basic test simple as it is.

As a new workflow file, or running a composite action in the go.yml workflow?

Example of how composite action would be used:

- name: Set up Go uses: actions/setup-go@v4 with: go-version: ${{ matrix.go }} id: go - 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 testing environment + uses: ./.github/actions/setup-filesystem + id: testenv - name: Test + working-directory: ${{ testenv.outputs.working_dir }} + env: + TEST_FILESYSTEM: ${{ matrix.os.filesystem }} + TEST_FILECOPYMETHOD: ${{ matrix.os.copymethod }} run: go test -v --tags=go${{ matrix.go }}

If a new workflow file, it would look mostly the same as go.yml looks right now.

New workflow is better

otiai10 · 2024-09-27T03:05:19Z

all_test.go

@@ -351,6 +362,9 @@ func TestOptions_PreserveOwner(t *testing.T) {
 }

 func TestOptions_CopyRateLimit(t *testing.T) {
+	if defaultCopyMethodName != "CopyBytes" {
+		t.Skipf("Can only test WrapReader with CopyBytes method, not %s", defaultCopyMethodName)
+	}


This doesn't seem the exception of test cases, but it is an expected behavior. In other words, this package should return error if users try to use CopyRateLimit when invalid copy-method is set.

This package should return error if users try to use CopyRateLimit when invalid copy-method is set.

Yes.

Sorry, I think I could have been more clear. This and the TestOptions_FS tests are skipped because the package does return an error for other FileCopyMethods, which is expected. That expected error makes these tests fail when FileCopyMethod != CopyBytes, which makes the CI workflow show as failed. I still want to re-use the same testing code for all FileCopyMethods, so I skip these tests to make the workflow pass.

Instead of skipping, do you want me to check that they correctly return the error? It would be more code and if branches, but has better coverage.

First of all, please do not say sorry, nor apologize. You're doing great contribution and I really appreciate your effort and passion. Being honest and frank is very important element to keep OSS clean.

correctly return the error

Well, yes, if it should do so, we should do so.

otiai10 · 2024-09-27T03:05:27Z

all_test.go

@@ -417,6 +431,10 @@ func TestOptions_OnFileError(t *testing.T) {
 }

 func TestOptions_FS(t *testing.T) {
+	if defaultCopyMethodName != "CopyBytes" {
+		t.Skipf("Can only test FS reading with CopyBytes method, not %s", defaultCopyMethodName)
+	}


otiai10 · 2024-09-27T03:07:59Z

all_test.go

@@ -17,8 +17,19 @@ import (
 //go:embed test/data/case18/assets
 var assets embed.FS

+func setupFileCopyMethod(m *testing.M) {
+	// Allow running all the tests with a different FileCopyMethod.
+	// We want to be able to have full coverage no matter the method.


Regarding you're using t.Skip, it is not the "coverage" you mean. Cases which should return error should return error. Users can avoid the case, but we should not skip the test. Please do not skip.

I'll change the t.Skip to check for an error instead, if that is better?

If it should return an error, let's do so.

otiai10 · 2024-09-27T05:02:28Z

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

I prefer simpleness and customizability, over implicit kindness.

eth-p · 2024-09-27T05:19:32Z

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

To clarify, this is the still default for all users of copy. I don't want to change it.

The "default" will never change outside the copy package tests. It's only configurable because that was the simplest/safest way to meet the goal of re-using the CopyBytes tests with other FileCopyMethods. Please let me know if there is a better option I missed, but I could not find a way to do that without giving up something. It was either:

Reduce maintainability — by having duplicate tests with different FileCopyMethods
Reduce simplicity of test code — by always adding the FileCopyMethod option to every call of Copy
Add test noise to non-test code — what I did here with the defaultCopyMethod

The other option would be to not re-use the CopyBytes tests, but that has different problems:

Edge cases tested in CopyBytes will not have tests for other FileCopyMethods
Tests can not catch when an other FileCopyMethod has an unexpected difference from CopyBytes

eth-p · 2024-09-27T06:02:24Z

all_test.go

 func setupFileCopyMethod(m *testing.M) {
 	// Allow running all the tests with a different FileCopyMethod.
 	// We want to be able to have full coverage no matter the method.
 	switch os.Getenv("TEST_FILECOPYMETHOD") {
 	case "CopyBytes":
 		defaultCopyMethod = CopyBytes
+		supportsWrapReaderOption = true


I could remove this line instead of setting it to true again if you want. I kind of like that it's explicit about what CopyBytes supports, though.

eth-p · 2024-09-27T06:04:29Z

I made the requested changes.

I also made a minor improvement by removing the defaultCopyMethodName variable from options.go and replacing it with variables in all_test.go to describe the capabilities of the current "default" FileCopyMethod.

otiai10 · 2024-09-27T08:56:58Z

options.go

@@ -134,6 +134,10 @@ type FileCopyMethod struct {
 	fcopy func(src, dest string, info os.FileInfo, opt Options) (err error, skipFile bool)
 }

+// The default FileCopyMethod.
+// This only is changed during tests.
+var defaultCopyMethod = CopyBytes


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

Users can set their opt.FileCopyMethod by theirselves, NOT by env or any other implicit mechanism.

The users of copy do not have any way to change this as a default. It can only be changed by developers of copy, and is only ever changed by the all_test.go code.

It's test-specific code, and I agree that's not ideal to have outside the *_test.go files. My earlier comment explained why it's needed, though.

Maybe naming it default is misleading? I could change it to unspecifiedFileCopyMethodOverride

I ended up removing this. I replaced it with a function that runs at the end of getDefaultOptions and replaces the FileCopyMethod.

otiai10 · 2024-09-27T08:59:48Z

options.go

@@ -149,7 +154,7 @@ func getDefaultOptions(src, dest string) Options {
 		Sync:              false,              // Do not sync
 		Specials:          false,              // Do not copy special files
 		PreserveTimes:     false,              // Do not preserve the modification time
-		FileCopyMethod:    CopyBytes,          // Copy by bytes
+		FileCopyMethod:    defaultCopyMethod,  // Copy by bytes, unless testing this package


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.

otiai10 · 2024-09-27T09:07:46Z

all_test.go

@@ -372,8 +386,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 supportsWrapReaderOption {


I totally disagree with this if.
If I were a user reading this test code,

first I would think "hmm, what's this supportsWrapReaderOption?"

second I would search supportsWrapReaderOption and find setupFileCopyMethod() and would think "now, what's TEST_FILECOPYMETHOD?"

third, I would find ${{ matrix.os.copymethod }} then I would know it's deeply coupled with filesystem

It is a long journey. We need to find more straightforward way so that users can easily understand.

I could move supportsWrapReadOption into the FileCopyMethod struct? That might be better.

.github/workflows/filecopymethod-test.yml

When we add support for reflinks, we need to make sure we are able to check that regressions aren't introduced. Explicitly testing on different filesystems allows us to do that.

In order to re-use existing test code for different FileCopyMethod implementations, we need some way to change which one the tests implicitly use (AKA: the default). Rather than wrapping the `Copy` function to inject the option or giving users the capability to change the default, we give the test code a hook for doing what it needs to do.

We want comprehensive testing, no matter the FileCopyMethod.

The only `FileCopyMethod` that supports this is CopyBytes.

otiai10 · 2024-09-30T12:39:16Z

all_test.go

@@ -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 {


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

otiai10 · 2024-09-30T12:39:34Z

all_test.go

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


otiai10 · 2024-09-30T12:39:50Z

copy_methods.go

@@ -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,


as mentioned above, we don't need these

otiai10 · 2024-09-30T12:43:44Z

all_test.go

@@ -17,8 +17,27 @@ import (
 //go:embed test/data/case18/assets
 var assets embed.FS

+var currentFileCopyMethod FileCopyMethod


We don't need this.

otiai10 · 2024-09-30T12:47:57Z

all_test.go

@@ -17,8 +17,27 @@ import (
 //go:embed test/data/case18/assets
 var assets embed.FS

+var currentFileCopyMethod FileCopyMethod
+
+func setupFileCopyMethod(m *testing.M) {


We don't need this func.

otiai10 · 2024-09-30T12:53:00Z

options.go

+// 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)


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

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.

otiai10 · 2024-10-02T00:56:55Z

Thank you for your input. I understand the points of conflict. I believe that DRY (Don't Repeat Yourself) principles are not as critical in testing. Please keep the code simple. Additionally, this pull request does not seem to address a specific issue. Therefore, I suggest we focus on #166 to solve a real problem. During the process of #166, I may or may not see the benefits of this pull request (#165).

At this moment, I’m not inclined to add logic that makes the test code overly DRY, from a readability perspective. I will not merge this.

eth-p force-pushed the feat-copymethod-api-testing branch from 680fd41 to 6e15bdb Compare August 25, 2024 01:28

eth-p mentioned this pull request Aug 25, 2024

feat: Add reflink FileCopyMethod (copy-on-write) #166

Open

4 tasks

eth-p force-pushed the feat-copymethod-api-testing branch from 6e15bdb to ad4b1e2 Compare August 25, 2024 01:35

eth-p force-pushed the feat-copymethod-api-testing branch 2 times, most recently from ac6bfb1 to 80e0cd0 Compare September 14, 2024 20:40

eth-p force-pushed the feat-copymethod-api-testing branch from 80e0cd0 to 8c37b2e Compare September 27, 2024 02:46

otiai10 requested changes Sep 27, 2024

View reviewed changes

eth-p force-pushed the feat-copymethod-api-testing branch from 8c37b2e to 4b06f26 Compare September 27, 2024 05:42

eth-p marked this pull request as draft September 27, 2024 05:45

eth-p force-pushed the feat-copymethod-api-testing branch 2 times, most recently from e528ce0 to cf524a2 Compare September 27, 2024 05:59

eth-p marked this pull request as ready for review September 27, 2024 06:00

eth-p requested a review from otiai10 September 27, 2024 06:00

eth-p commented Sep 27, 2024

View reviewed changes

otiai10 requested changes Sep 27, 2024

View reviewed changes

eth-p force-pushed the feat-copymethod-api-testing branch 5 times, most recently from 1ab8e8a to bd3c159 Compare September 27, 2024 16:29

eth-p added 3 commits September 27, 2024 09:31

ci: Add new workflow to run tests under different filesystems

aea1160

When we add support for reflinks, we need to make sure we are able to check that regressions aren't introduced. Explicitly testing on different filesystems allows us to do that.

feat: Add capability info to FileCopyMethod struct

4e36e6f

eth-p force-pushed the feat-copymethod-api-testing branch from bd3c159 to c2db18d Compare September 27, 2024 16:31

eth-p requested a review from otiai10 September 27, 2024 16:32

eth-p added 4 commits September 27, 2024 09:36

test: Add env var to run all tests under different FileCopyMethod

ae5d1d1

We want comprehensive testing, no matter the FileCopyMethod.

test: Expect error when WrapReader not supported by FileCopyMethod

3314eec

The only `FileCopyMethod` that supports this is CopyBytes.

test: Expect error when FS not supported by FileCopyMethod

1c0a1c6

The only `FileCopyMethod` that supports this is CopyBytes.

ci(filecopymethod-test): Add FileCopyMethod to the matrix

207b8f7

eth-p force-pushed the feat-copymethod-api-testing branch from c2db18d to 207b8f7 Compare September 27, 2024 16:36

otiai10 requested changes Sep 30, 2024

View reviewed changes

otiai10 closed this Oct 2, 2024

test: Add testing for FileCopyMethod #165

test: Add testing for FileCopyMethod #165

Conversation

eth-p commented Aug 25, 2024 • edited Loading

Reason

Details

Example

otiai10 commented Sep 26, 2024

eth-p commented Sep 26, 2024 • edited Loading

otiai10 commented Sep 27, 2024

eth-p commented Sep 27, 2024

Choose a reason for hiding this comment

eth-p Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eth-p Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eth-p Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eth-p Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

otiai10 commented Sep 27, 2024

eth-p commented Sep 27, 2024 • edited Loading

eth-p Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

eth-p commented Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

eth-p Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eth-p Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

What we tried before

Wrapping the Copy function inside tests

Creating a defaultCopyMethod that only changes inside tests

What we could do

Have a global variable in all_test.go

Copy the code of tests

Only making new tests

Only making new tests

Go build tags

otiai10 commented Oct 2, 2024

eth-p commented Aug 25, 2024 •

edited

Loading

eth-p commented Sep 26, 2024 •

edited

Loading

eth-p Sep 27, 2024 •

edited

Loading

eth-p Sep 27, 2024 •

edited

Loading

eth-p Sep 27, 2024 •

edited

Loading

eth-p Sep 27, 2024 •

edited

Loading

eth-p commented Sep 27, 2024 •

edited

Loading

eth-p Sep 27, 2024 •

edited

Loading

eth-p commented Sep 27, 2024 •

edited

Loading

eth-p Sep 27, 2024 •

edited

Loading

eth-p Sep 30, 2024 •

edited

Loading

Creating a `defaultCopyMethod` that only changes inside tests

Have a global variable in `all_test.go`