Repository: uber-go/fx
Pull request #784 (2021-09-04 08:31:24)

Make Populate return errorOption allowing ValidateApp to recognise wireup problems
This PR solves issue #762 that I've reported earlier. 

This slightly changes the internal behaviour of `Populate`, instead of returning a dedicated custom `invokeErr` option it returns a standard `errorOption`. This allows the `ValidateApp()` helper to recognise wire-up issues that arise from incorrectly configured `Populate` options. This wasn't possible earlier because `ValidateApp` sets `DryRun=true` for the `dig` container, and that makes it ignore and not execute any invocations, so the `invokeErr` was never called and the related errors could not be discovered in the unit tests relying on `ValidateApp`.

Using `errorOption` sets the `App.err` explicitly on applying option (rather than on calling invocations), so it allows to surface errors produced by `Populate` in dry-run mode without calling the invocations.

This also makes the same change to the deprecated `Extract` function, since I'm not sure when/if it will be removed.

codecov[bot]: # [Codecov](https://codecov.io/gh/uber-go/fx/pull/784?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) Report
> Merging [#784](https://codecov.io/gh/uber-go/fx/pull/784?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) (2300f86) into [master](https://codecov.io/gh/uber-go/fx/commit/9b7de2d2224837121de68b61b5d4ed0d1fd7bf97?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) (9b7de2d) will **decrease** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/uber-go/fx/pull/784/graphs/tree.svg?width=650&height=150&src=pr&token=PYY9PSfaC6&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go)](https://codecov.io/gh/uber-go/fx/pull/784?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go)

```diff
@@            Coverage Diff             @@
##           master     #784      +/-   ##
==========================================
- Coverage   97.93%   97.93%   -0.01%     
==========================================
  Files          24       24              
  Lines         873      870       -3     
==========================================
- Hits          855      852       -3     
  Misses         13       13              
  Partials        5        5              
```


| [Impacted Files](https://codecov.io/gh/uber-go/fx/pull/784?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go) | Coverage Δ | |
|---|---|---|
| [extract.go](https://codecov.io/gh/uber-go/fx/pull/784/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-ZXh0cmFjdC5nbw==) | `100.00% <100.00%> (ø)` | |
| [populate.go](https://codecov.io/gh/uber-go/fx/pull/784/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go#diff-cG9wdWxhdGUuZ28=) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/uber-go/fx/pull/784?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/uber-go/fx/pull/784?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go). Last update [9b7de2d...2300f86](https://codecov.io/gh/uber-go/fx/pull/784?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=uber-go).


Patch:
diff --git a/extract.go b/extract.go
index 8fb34a0..b9b7393 100644
--- a/extract.go
+++ b/extract.go
@@ -39,7 +39,7 @@ func Extract(target interface{}) Option {
 	v := reflect.ValueOf(target)
 
 	if t := v.Type(); t.Kind() != reflect.Ptr || t.Elem().Kind() != reflect.Struct {
-		return invokeErr(fmt.Errorf("Extract expected a pointer to a struct, got a %v", t))
+		return Error(fmt.Errorf("Extract expected a pointer to a struct, got a %v", t))
 	}
 
 	v = v.Elem()
diff --git a/populate.go b/populate.go
index 1c503c5..4195e80 100644
--- a/populate.go
+++ b/populate.go
@@ -38,11 +38,11 @@ func Populate(targets ...interface{}) Option {
 	targetTypes := make([]reflect.Type, len(targets))
 	for i, t := range targets {
 		if t == nil {
-			return invokeErr(fmt.Errorf("failed to Populate: target %v is nil", i+1))
+			return Error(fmt.Errorf("failed to Populate: target %v is nil", i+1))
 		}
 		rt := reflect.TypeOf(t)
 		if rt.Kind() != reflect.Ptr {
-			return invokeErr(fmt.Errorf("failed to Populate: target %v is not a pointer type, got %T", i+1, t))
+			return Error(fmt.Errorf("failed to Populate: target %v is not a pointer type, got %T", i+1, t))
 		}
 
 		targetTypes[i] = reflect.TypeOf(t).Elem()
@@ -65,9 +65,3 @@ func Populate(targets ...interface{}) Option {
 	})
 	return Invoke(fn.Interface())
 }
-
-func invokeErr(err error) Option {
-	return Invoke(func() error {
-		return err
-	})
-}


Test patch:
diff --git a/app_test.go b/app_test.go
index de64156..ecaa871 100644
--- a/app_test.go
+++ b/app_test.go
@@ -65,6 +65,18 @@ func NewSpied(opts ...Option) (*App, *fxlog.Spy) {
 	return New(opts...), spy
 }
 
+func validateTestApp(tb testing.TB, opts ...Option) error {
+	testOpts := []Option{
+		// Provide both: Logger and WithLogger so that if the test
+		// WithLogger fails, we don't pollute stderr.
+		Logger(fxtest.NewTestPrinter(tb)),
+		WithLogger(func() fxevent.Logger { return fxtest.NewTestLogger(tb) }),
+	}
+	opts = append(testOpts, opts...)
+
+	return ValidateApp(opts...)
+}
+
 func TestNewApp(t *testing.T) {
 	t.Run("ProvidesLifecycleAndShutdowner", func(t *testing.T) {
 		var (
diff --git a/extract_test.go b/extract_test.go
index 4f4fe3d..3abe9ab 100644
--- a/extract_test.go
+++ b/extract_test.go
@@ -59,6 +59,27 @@ func TestExtract(t *testing.T) {
 		}
 	})
 
+	t.Run("ValidateApp", func(t *testing.T) {
+		tests := []interface{}{
+			3,
+			func() {},
+			struct{}{},
+			struct{ Foo *bytes.Buffer }{},
+		}
+
+		for _, tt := range tests {
+			t.Run(fmt.Sprintf("%T", tt), func(t *testing.T) {
+				err := validateTestApp(t,
+					Provide(func() *bytes.Buffer { return &bytes.Buffer{} }),
+					Extract(&tt),
+				)
+
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), "Extract expected a pointer to a struct")
+			})
+		}
+	})
+
 	t.Run("Empty", func(t *testing.T) {
 		new1 := func() *type1 { panic("new1 must not be called") }
 		new2 := func() *type2 { panic("new2 must not be called") }
diff --git a/populate_test.go b/populate_test.go
index d240cbf..3eb83c2 100644
--- a/populate_test.go
+++ b/populate_test.go
@@ -28,6 +28,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
 	. "go.uber.org/fx"
 	"go.uber.org/fx/fxtest"
 )
@@ -244,3 +245,73 @@ func TestPopulateErrors(t *testing.T) {
 		assert.Contains(t, app.Err().Error(), tt.wantErr)
 	}
 }
+
+func TestPopulateValidateApp(t *testing.T) {
+	type t1 struct{}
+	type container struct {
+		In
+
+		T1 t1
+	}
+	type containerNoIn struct {
+		T1 t1
+	}
+	fn := func() {}
+	var v *t1
+
+	tests := []struct {
+		msg     string
+		opts    []interface{}
+		wantErr string
+	}{
+		{
+			msg:     "inline value",
+			opts:    []interface{}{t1{}},
+			wantErr: "not a pointer",
+		},
+		{
+			msg:     "container value",
+			opts:    []interface{}{container{}},
+			wantErr: "not a pointer",
+		},
+		{
+			msg:     "container pointer without fx.In",
+			opts:    []interface{}{&containerNoIn{}},
+			wantErr: "missing type: fx_test.containerNoIn",
+		},
+		{
+			msg:     "function",
+			opts:    []interface{}{fn},
+			wantErr: "not a pointer",
+		},
+		{
+			msg:     "function pointer",
+			opts:    []interface{}{&fn},
+			wantErr: "missing type: func()",
+		},
+		{
+			msg:     "invalid last argument",
+			opts:    []interface{}{&v, t1{}},
+			wantErr: "target 2 is not a pointer type",
+		},
+		{
+			msg:     "nil argument",
+			opts:    []interface{}{&v, nil, &v},
+			wantErr: "target 2 is nil",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.msg, func(t *testing.T) {
+			testOpts := []Option{
+				NopLogger,
+				Provide(func() *t1 { return &t1{} }),
+				Populate(tt.opts...),
+			}
+
+			err := validateTestApp(t, testOpts...)
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), tt.wantErr)
+		})
+	}
+}