Open: expose error cause, test for fs.ErrNotExist (#1149)

This is a prefactor for Windows support (#621). TestOpen currently relies on a hardcoded error "no such file or directory" if a file is not found. However, this error message is OS-specific. We can now rely on `errors.Is(err, fs.ErrNotExist)` if we wrap errors using %w instead of %v. Co-authored-by: Abhinav Gupta <[email protected]>
uber-go · Aug 16, 2022 · 19a5d8a · 19a5d8a
1 parent bdd673d
commit 19a5d8a
Show file tree

Hide file tree

Showing 7 changed files with 139 additions and 41 deletions.
diff --git a/benchmarks/go.sum b/benchmarks/go.sum
@@ -60,11 +60,14 @@ github.com/smartystreets/assertions v1.0.0/go.mod h1:kHHU4qYBaI3q23Pp3VPrmWhuIUr
 github.com/smartystreets/go-aws-auth v0.0.0-20180515143844-0c1422d1fdb9/go.mod h1:SnhjPscd9TpLiy1LpzGSKh3bXCfxxXuqd9xmQJy3slM=
 github.com/smartystreets/gunit v1.0.0/go.mod h1:qwPWnhz6pn0NnRBP++URONOVyNkPyr4SauJk4cUOwJs=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
+github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
 github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
-github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY=
 github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.8.0 h1:pSgiaMZlXftHpm5L7V1+rVB+AZJydKsMxsQBIJw4PKk=
+github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
 github.com/tj/assert v0.0.0-20171129193455-018094318fb0/go.mod h1:mZ9/Rh9oLWpLLDRpvE+3b7gP/C2YyLFYxNmcLnPTMe0=
 github.com/tj/assert v0.0.3 h1:Df/BlaZ20mq6kuai7f5z2TvPFiwC3xaWJSDQNiIS3Rk=
 github.com/tj/assert v0.0.3/go.mod h1:Ne6X72Q+TB1AteidzQncjw9PabbMp4PBMZ1k+vd1Pvk=

diff --git a/go.mod b/go.mod
@@ -5,7 +5,7 @@ go 1.18
 require (
 	github.com/benbjohnson/clock v1.1.0
 	github.com/pkg/errors v0.8.1
-	github.com/stretchr/testify v1.7.0
+	github.com/stretchr/testify v1.8.0
 	go.uber.org/atomic v1.7.0
 	go.uber.org/goleak v1.1.11
 	go.uber.org/multierr v1.6.0

diff --git a/go.sum b/go.sum
@@ -13,9 +13,12 @@ github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINE
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
+github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
-github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY=
 github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.8.0 h1:pSgiaMZlXftHpm5L7V1+rVB+AZJydKsMxsQBIJw4PKk=
+github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
 github.com/yuin/goldmark v1.3.5/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1Zlc8k=
 go.uber.org/atomic v1.7.0 h1:ADUqmZGgLDDfbSL9ZmPxKTybcoEYHgpYfELNoN+7hsw=
 go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=

diff --git a/writer.go b/writer.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2016 Uber Technologies, Inc.
+// Copyright (c) 2016-2022 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal
@@ -70,7 +70,7 @@ func open(paths []string) ([]zapcore.WriteSyncer, func(), error) {
 	for _, path := range paths {
 		sink, err := newSink(path)
 		if err != nil {
-			openErr = multierr.Append(openErr, fmt.Errorf("couldn't open sink %q: %v", path, err))
+			openErr = multierr.Append(openErr, fmt.Errorf("open sink %q: %w", path, err))
 			continue
 		}
 		writers = append(writers, sink)

diff --git a/writer_test.go b/writer_test.go
@@ -1,4 +1,4 @@
-// Copyright (c) 2016 Uber Technologies, Inc.
+// Copyright (c) 2016-2022 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal
@@ -23,14 +23,15 @@ package zap
 import (
 	"errors"
 	"io"
+	"io/fs"
 	"net/url"
 	"os"
 	"path/filepath"
-	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/multierr"
 	"go.uber.org/zap/zapcore"
 )
 
@@ -50,55 +51,95 @@ func TestOpenNoPaths(t *testing.T) {
 func TestOpen(t *testing.T) {
 	tempName := filepath.Join(t.TempDir(), "test.log")
 	assert.False(t, fileExists(tempName))
-	require.True(t, strings.HasPrefix(tempName, "/"), "Expected absolute temp file path.")
+	require.True(t, filepath.IsAbs(tempName), "Expected absolute temp file path.")
 
 	tests := []struct {
+		msg   string
 		paths []string
-		errs  []string
 	}{
-		{[]string{"stdout"}, nil},
-		{[]string{"stderr"}, nil},
-		{[]string{tempName}, nil},
-		{[]string{"file://" + tempName}, nil},
-		{[]string{"file://localhost" + tempName}, nil},
-		{[]string{"/foo/bar/baz"}, []string{"open /foo/bar/baz: no such file or directory"}},
-		{[]string{"file://localhost/foo/bar/baz"}, []string{"open /foo/bar/baz: no such file or directory"}},
 		{
-			paths: []string{"stdout", "/foo/bar/baz", tempName, "file:///baz/quux"},
-			errs: []string{
-				"open /foo/bar/baz: no such file or directory",
-				"open /baz/quux: no such file or directory",
-			},
+			msg:   "stdout",
+			paths: []string{"stdout"},
+		},
+		{
+			msg:   "stderr",
+			paths: []string{"stderr"},
+		},
+		{
+			msg:   "temp file path only",
+			paths: []string{tempName},
+		},
+		{
+			msg:   "temp file file scheme",
+			paths: []string{"file://" + tempName},
+		},
+		{
+			msg:   "temp file with file scheme and host localhost",
+			paths: []string{"file://localhost" + tempName},
 		},
-		{[]string{"file:///stderr"}, []string{"open /stderr:"}},
-		{[]string{"file:///stdout"}, []string{"open /stdout:"}},
-		{[]string{"file://host01.test.com" + tempName}, []string{"empty or use localhost"}},
-		{[]string{"file://rms@localhost" + tempName}, []string{"user and password not allowed"}},
-		{[]string{"file://localhost" + tempName + "#foo"}, []string{"fragments not allowed"}},
-		{[]string{"file://localhost" + tempName + "?foo=bar"}, []string{"query parameters not allowed"}},
-		{[]string{"file://localhost:8080" + tempName}, []string{"ports not allowed"}},
 	}
 
 	for _, tt := range tests {
-		_, cleanup, err := Open(tt.paths...)
-		if err == nil {
-			defer cleanup()
-		}
+		t.Run(tt.msg, func(t *testing.T) {
+			_, cleanup, err := Open(tt.paths...)
+			if err == nil {
+				defer cleanup()
+			}
 
-		if len(tt.errs) == 0 {
 			assert.NoError(t, err, "Unexpected error opening paths %v.", tt.paths)
-		} else {
-			msg := err.Error()
-			for _, expect := range tt.errs {
-				assert.Contains(t, msg, expect, "Unexpected error opening paths %v.", tt.paths)
-			}
-		}
+		})
 	}
 
 	assert.True(t, fileExists(tempName))
 	os.Remove(tempName)
 }
 
+func TestOpenPathsNotFound(t *testing.T) {
+	tempName := filepath.Join(t.TempDir(), "test.log")
+
+	tests := []struct {
+		msg               string
+		paths             []string
+		wantNotFoundPaths []string
+	}{
+		{
+			msg:               "missing path",
+			paths:             []string{"/foo/bar/baz"},
+			wantNotFoundPaths: []string{"/foo/bar/baz"},
+		},
+		{
+			msg:               "missing file scheme url with host localhost",
+			paths:             []string{"file://localhost/foo/bar/baz"},
+			wantNotFoundPaths: []string{"/foo/bar/baz"},
+		},
+		{
+			msg:   "multiple paths",
+			paths: []string{"stdout", "/foo/bar/baz", tempName, "file:///baz/quux"},
+			wantNotFoundPaths: []string{
+				"/foo/bar/baz",
+				"/baz/quux",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.msg, func(t *testing.T) {
+			_, cleanup, err := Open(tt.paths...)
+			if !assert.Error(t, err, "Open must fail.") {
+				cleanup()
+				return
+			}
+
+			errs := multierr.Errors(err)
+			require.Len(t, errs, len(tt.wantNotFoundPaths))
+			for i, err := range errs {
+				assert.ErrorIs(t, err, fs.ErrNotExist)
+				assert.ErrorContains(t, err, tt.wantNotFoundPaths[i], "missing path in error")
+			}
+		})
+	}
+}
+
 func TestOpenRelativePath(t *testing.T) {
 	const name = "test-relative-path.txt"
 
@@ -136,6 +177,54 @@ func TestOpenFails(t *testing.T) {
 	}
 }
 
+func TestOpenOtherErrors(t *testing.T) {
+	tempName := filepath.Join(t.TempDir(), "test.log")
+
+	tests := []struct {
+		msg     string
+		paths   []string
+		wantErr string
+	}{
+		{
+			msg:     "file with unexpected host",
+			paths:   []string{"file://host01.test.com" + tempName},
+			wantErr: "empty or use localhost",
+		},
+		{
+			msg:     "file with user on localhost",
+			paths:   []string{"file://rms@localhost" + tempName},
+			wantErr: "user and password not allowed",
+		},
+		{
+			msg:     "file url with fragment",
+			paths:   []string{"file://localhost" + tempName + "#foo"},
+			wantErr: "fragments not allowed",
+		},
+		{
+			msg:     "file url with query",
+			paths:   []string{"file://localhost" + tempName + "?foo=bar"},
+			wantErr: "query parameters not allowed",
+		},
+		{
+			msg:     "file with port",
+			paths:   []string{"file://localhost:8080" + tempName},
+			wantErr: "ports not allowed",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.msg, func(t *testing.T) {
+			_, cleanup, err := Open(tt.paths...)
+			if !assert.Error(t, err, "Open must fail.") {
+				cleanup()
+				return
+			}
+
+			assert.ErrorContains(t, err, tt.wantErr, "Unexpected error opening paths %v.", tt.paths)
+		})
+	}
+}
+
 type testWriter struct {
 	expected string
 	t        testing.TB

diff --git a/zapgrpc/internal/test/go.mod b/zapgrpc/internal/test/go.mod
@@ -3,7 +3,7 @@ module go.uber.org/zap/zapgrpc/internal/test
 go 1.17
 
 require (
-	github.com/stretchr/testify v1.7.0
+	github.com/stretchr/testify v1.8.0
 	go.uber.org/zap v1.16.0
 	google.golang.org/grpc v1.42.0
 )

diff --git a/zapgrpc/internal/test/go.sum b/zapgrpc/internal/test/go.sum
@@ -65,10 +65,13 @@ github.com/rogpeppe/go-internal v1.6.1/go.mod h1:xXDCJY+GAPziupqXw64V24skbSoqbTE
 github.com/rogpeppe/go-internal v1.8.0 h1:FCbCCtXNOY3UtUuHUYaghJg4y7Fd14rXifAYUAtL9R8=
 github.com/rogpeppe/go-internal v1.8.0/go.mod h1:WmiCO8CzOY8rg0OYDC4/i/2WRWAB6poM+XZ2dLUbcbE=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
+github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
 github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA=
-github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY=
 github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.8.0 h1:pSgiaMZlXftHpm5L7V1+rVB+AZJydKsMxsQBIJw4PKk=
+github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
 github.com/yuin/goldmark v1.3.5/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1Zlc8k=
 go.opentelemetry.io/proto/otlp v0.7.0/go.mod h1:PqfVotwruBrMGOCsRd/89rSnXhoiJIqeYNgFYFoEGnI=
 go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=