Skip to content

testing/fstest: TestFS should return an unwrappable error #63675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
dolmen opened this issue Oct 22, 2023 · 4 comments
Closed

testing/fstest: TestFS should return an unwrappable error #63675

dolmen opened this issue Oct 22, 2023 · 4 comments
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.23

Comments

@dolmen
Copy link
Contributor

dolmen commented Oct 22, 2023 •
edited
Loading

testing/fstest.TestFS reports a single error which is the concatenation of all errors. This doesn't allow to analyze the errors by unwraping using errors.Is or errors.As or calling Unwrap() []error.

See TestFS code: https://github.com/golang/go/blob/master/src/testing/fstest/testfs.go#L107

Instead TestFS should use errors.Join or a custom error that implements the Unwrap() []error method like errors.Join.

What version of Go are you using (go version)?

$ go version
go version go1.21.3 darwin/amd64

Does this issue reproduce with the latest release?

Yes, on master branch.

Check code of the fsTester.errorf method:
https://github.com/golang/go/blob/master/src/testing/fstest/testfs.go#L107

What did you do?

I'm testing a filesystem that enforces permissions: https://github.com/dolmen-go/sqlar

I applied TestFS to a filesystem that returns fs.ErrPermission for ReadDir on multiple directories.

What did you expect to see?

I would like to analyze the errors using errors.Is to ignore expected fs.ErrPermission errors.

What did you see instead?

I got a big string containing the error messages concatenated.
@dolmen dolmen changed the title testing/fstest: TestFS should use errors.Join to report errors testing/fstest: TestFS should retun an unwrappable error Oct 23, 2023
@dolmen
Copy link
Contributor Author

dolmen commented Oct 23, 2023

I'm working on a patch.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/537015 mentions this issue: testing/fstest: return structured errors in TestFS

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 23, 2023
@dolmen dolmen changed the title testing/fstest: TestFS should retun an unwrappable error testing/fstest: TestFS should return an unwrappable error Oct 24, 2023
dolmen added a commit to dolmen-go/sqlar that referenced this issue Oct 24, 2023
@dolmen
sqlarfs: add TestMulti to check permissions enforcement
4024f58 
Add testdata/perms.sqlar to check the permissions system.

This allowed to discover two issues in testing/fstest.TestFS:
- golang/go#63675 errors must be unwrappable
- golang/go#63707 relax check on ReadDir if
  mode & 0444 != 0444
dolmen added a commit to dolmen-go/sqlar that referenced this issue Oct 24, 2023
@dolmen
sqlarfs: add TestPerms to check permissions enforcement
21e94a4 
Add testdata/perms.sqlar to check the permissions system.

This allowed to discover two issues in testing/fstest.TestFS:
- golang/go#63675 errors must be unwrappable
- golang/go#63707 relax check on ReadDir if
  mode & 0444 != 0444
@dolmen dolmen mentioned this issue Oct 24, 2023
Merged
@dolmen
Copy link
Contributor Author

dolmen commented Dec 4, 2023 •
edited
Loading

Reviews of CL #537015 would be welcome.

@dolmen
Copy link
Contributor Author

dolmen commented Mar 30, 2024

Here is the workaround I had to implement in my sqlarfs testsuite when testing for fs.ErrPermission: string.Contains applied to err.Error.
https://github.com/dolmen-go/sqlar/blob/main/sqlarfs/sqlarfs_test.go#L178

		var errs interface{ Unwrap() []error }
		switch {
		case err == nil: // ignore
		case errors.As(err, &errs):
			for _, err := range errs.Unwrap() {
				if errors.Is(err, fs.ErrPermission) {
					t.Logf("%T: %[1]q", err)
				} else {
					t.Fatal(err)
				}
			}
		case errors.Is(err, fs.ErrPermission):
			t.Logf("%T: %[1]q", err)
		default:
			t.Logf("%T: %[1]q", err)
			if strings.Contains(err.Error(), fs.ErrPermission.Error()) {
				t.Logf("%T: %[1]q", err)
				t.Log("Fallback to error message check")
				t.Skip("Skip. See https://github.com/golang/go/issues/63675")
			}
			t.Fatalf("%T: %[1]q", err)
		}

My CL 537015 is still waiting.

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. FixPending Issues that have a fix which has not yet been reviewed or submitted. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 5, 2024
@dmitshur dmitshur added this to the Go1.23 milestone Apr 5, 2024
@golang golang locked and limited conversation to collaborators Apr 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FixPending Issues that have a fix which has not yet been reviewed or submitted. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.23
Development

No branches or pull requests
4 participants
@dolmen @dmitshur @bcmills @gopherbot