[1/2] Add tests for user-facing behavior #70

akshayjshah · 2017-05-26T22:29:50Z

☮️ 🙏 😇 🙇 I come in peace 🙇😇 🙏 ☮️

TL;DR: This PR adds integration tests, which fail. Fixing the tests requires
enough refactoring that I separated it into in a follow-on PR.

As I was working on Fx modules that use dig, I noticed some behavior that
surprised me. In exploring dig's code, I realized that many of my assumptions
about how the library should behave aren't covered by dig's testing suite.

In this PR, I've added a bunch of end-to-end tests in the top-level dig package.
Those tests only use the public Provide and Invoke APIs to verify that the
behaviors Fx needs work correctly. I haven't altered any of dig's production
code.

Unfortunately, these tests exposed a number of bugs:

$ go test . | grep FAIL

    # Container.Provide(nil) panics.
--- FAIL: TestCantProvideUntypedNil (0.00s)

    # The container allows us to provide errors, but only via some code paths.
--- FAIL: TestCantProvideErrors (0.00s)

    # All types in the graph must have their deps satisfied, even if nobody's
    # using them.
--- FAIL: TestIncompleteGraphIsOkay (0.00s)

--- FAIL: TestEndToEndSuccess (0.00s)
    # We can't provide slices or maps from standard constructor functions, but
    # we can provide instances. Elaborate constructors like `func() []Foo, int,
    # error` also work correctly.
    --- FAIL: TestEndToEndSuccess/slice_constructor (0.00s)
    --- FAIL: TestEndToEndSuccess/map_constructor (0.00s)

    # We can't provide both []*Foo and *Foo, since dig considers them the same
    # type.
    --- FAIL: TestEndToEndSuccess/collections_and_instances_of_same_type (0.00s)

This is concerning to me, since dig has 89% code coverage. Fixing these bugs and
structuring the code so that we can confidently refactor without breaking
user-facing behavior was a larger change, so I've broken it out into a separate
PR.

TL;DR: This PR adds integration tests, which fail. Fixing the tests is a much larger change made in a separate PR. As I was working on some Fx modules that use dig, I noticed some behavior that surprised me. In exploring dig's code, I realized that many of my assumptions about how the library should behave aren't covered by dig's testing suite. In this PR, I've added a bunch of end-to-end tests in the top-level dig package. Those tests *only* use the public `Provide` and `Invoke` APIs to verify that the behaviors Fx needs work correctly. I haven't altered any of dig's production code. Unfortunately, these tests exposed a number of bugs: ``` $ go test . | grep FAIL # Container.Provide(nil) panics. --- FAIL: TestCantProvideUntypedNil (0.00s) # The container allows us to provide errors, but via some code paths. --- FAIL: TestCantProvideErrors (0.00s) # All types in the graph must have their deps satisfied, even if nobody's # using them. --- FAIL: TestIncompleteGraphIsOkay (0.00s) # Container doesn't detect cycles. --- FAIL: TestProvideCycleFails (0.00s) --- FAIL: TestEndToEndSuccess (0.00s) # We can't provide slices or maps from standard constructor functions, but # we can provide instances. Elaborate constructors like `func() []Foo, int, # error` also work correctly. --- FAIL: TestEndToEndSuccess/slice_constructor (0.00s) --- FAIL: TestEndToEndSuccess/map_constructor (0.00s) # We can't provide both []Foo and Foo, since dig considers them the same # type. --- FAIL: TestEndToEndSuccess/collections_and_instances_of_same_type (0.00s) ``` This is concerning to me, since dig has 89% code coverage. Fixing these bugs and structuring the code so that we can confidently refactor without breaking user-facing behavior was a larger change, so I've broken it out into a separate PR.

HelloGrayson

Beautifully written tests, thanks @akshayjshah - these (once passing) gives me confidence that dig does what it needs to for Fx, and no more.

ghost · 2017-05-30T16:50:05Z

dig_test.go

+		c := New()
+		bufs := [1]*bytes.Buffer{{}}
+		require.NoError(t, c.Provide(bufs), "provide failed")
+		require.NoError(t, c.Invoke(func(bs [1]*bytes.Buffer) {


Do we want to work with arrays as pointer or values in dig? For me it feels like it should be a pointer to avoid copying, etc. Drawing a parallel to other value types: structs, we do require pointers for them.

IMO, we want to work with whatever is provided. It's up to the user to decide which they want.

ghost · 2017-05-30T16:55:41Z

dig_test.go

+		require.NoError(t, c.Invoke(consumer), "invoke failed")
+	})
+
+	t.Run("collections and instances of same type", func(t *testing.T) {


I think I am missing something in this test, when a slice and a pointer can be mixed?

The test points out that dig's pointer and map support is currently a little broken - it's not possible to provide both []*Foo and *Foo. Obviously, we should support providing both.

ghost · 2017-05-30T17:18:15Z

dig_test.go

+	c := New()
+
+	assert.Error(t, c.Provide(func() error { return errors.New("foo") }))
+	// TODO: This is another case where we're losing type information, which


What about a regular type comparison, that we are using:
https://github.com/uber-go/dig/blob/master/container.go#L71
Is it a bug then? https://play.golang.org/p/N0c4JiwR4W

What makes me to think about adding a test, that has a struct, that implements the error interface..

The type comparison works fine for constructors that explicitly return error. It doesn't work if the constructor returns a type that implements the error interface. It definitely doesn't work for instances, since we lose interface information by passing instances to Provide as interface{} (see https://research.swtch.com/interfaces for details). We could drop all types that implement error, but that's a larger change.

The intention of these two PRs isn't to fix all bugs in dig, but to get to a place where everyone knows what dig's behavior actually is.

ghost · 2017-05-30T17:19:26Z

dig_test.go

+	type A struct{}
+	type B struct{}
+	type C struct{}
+	newA := func(C) A { return A{} }


Looks like the test fails because returned types are not pointers..

Good point! Changed.

prashantv

+1 on test methadology. I think we should validate the results a little more thoroughly though.

prashantv · 2017-05-30T20:42:35Z

dig_test.go

+)
+
+func TestEndToEndSuccess(t *testing.T) {
+	t.Parallel()


Do these tests take long enough to make Parallel worth it? It makes debugging failures a little trickier so I wouldn't add it unless we're seeing a significant win.

Not really, but all the existing tests were marked parallel. In the interest of minimizing change, I stuck with that convention.

prashantv · 2017-05-30T20:42:59Z

dig_test.go

+		c := New()
+		require.NoError(t, c.Provide(&bytes.Buffer{}), "provide failed")
+		require.NoError(t, c.Invoke(func(b *bytes.Buffer) {
+			require.NotNil(t, b, "invoke got nil buffer")


since you're providing a specific value, can you assert that the value that got passed in here matches?

prashantv · 2017-05-30T20:44:14Z

dig_test.go

+			return &bytes.Buffer{}
+		}), "provide failed")
+		require.NoError(t, c.Invoke(func(b *bytes.Buffer) {
+			require.NotNil(t, b, "invoke got nil buffer")


can we assert that Invoke gets passed in the same value the constructor returned

prashantv · 2017-05-30T20:45:39Z

dig_test.go

+	t.Run("struct", func(t *testing.T) {
+		t.Skip("Not yet supported.")
+		c := New()
+		buf := &bytes.Buffer{}


why create a pointer only to dereference it below? you can just do var buf bytes.Buffer or buf := bytes.Buffer{}

Yep, good point.

prashantv · 2017-05-30T20:46:50Z

dig_test.go

+
+	t.Run("slice", func(t *testing.T) {
+		c := New()
+		bufs := []*bytes.Buffer{{}, {}}


if we're testing a slice, can we keep it simpler and do something like []int{1,2} -- or did you want to test slice of struct pointers specifically?

using a simple slice and verifying the contents of the slice seems simpler to understand and provides more validation.

I'd prefer not to, since there's an ongoing discussion about whether dig should allow primitive values at all. I'd prefer to stick to some more-realistic type here, so that future changes don't also need to alter these high-level types.

This test should also check that invoke receives the same pointer that provide returned, though.

prashantv · 2017-05-30T20:48:41Z

dig_test.go

+}
+
+func TestCantProvideUntypedNil(t *testing.T) {
+	t.Fatalf("Calling Container.Provide(nil) crashes the main testing thread. Failing early.")


why do some tests use Skip while some use Fatal? Seems like this should probably be skip as well?

prashantv · 2017-05-30T20:50:19Z

dig_test.go

+	t.Parallel()
+	c := New()
+
+	assert.Error(t, c.Provide(func() error { return errors.New("foo") }))


Just curious about this behaviour -- what happens if the func returns a type that is not error, but the type returned implements error?

Since the check is against the exact error type, that'll just work. Added a test for it.

prashantv · 2017-05-30T20:53:03Z

dig_test.go

+	t.Parallel()
+	c := New()
+	assert.NoError(t, c.Provide(func() *bytes.Buffer { return nil }))
+	assert.Error(t, c.Provide(func() *bytes.Buffer { return nil }))


can you verify that it fails even if the value is provided. this might need to be a loop test like:

provideArgs := []interface{}{ func() *bytes.Buffer { return nil }, func() (*bytes.Buffer, error) { return nil, nil }, &bytes.Buffer{}, } for _, firstProvide := range provideArgs { assert.NoError(t, c.Provide(firstProvide), ...) for _, secondProvide := range provideArgs { assert.Error(t, c.Provide(secondProvide), ...) } }

that way we ensure that once a type is known (regardless of how it was provided), other provides fail.

prashantv · 2017-05-30T20:54:52Z

dig_test.go

+	c := New()
+	calls := 0
+	buf := &bytes.Buffer{}
+	require.NoError(t, c.Provide(func() *bytes.Buffer { return buf }))


as an extra safety, can we have the provided func increment a counter and ensure that the value is 0 initially, and then 1 the loop

prashantv · 2017-05-31T18:46:04Z

dig_test.go

+			for _, second := range provideArgs {
+				assert.Error(t, c.Provide(second), "second provide must fail")
+			}
+


super nit: remove blank before }

See #70 for context. This PR follows up on the previous test-only diff. It removes all pre-existing tests, fixes the integration tests, and finally removes all features not required to make the end-to-end tests pass. Along the way, it does some fairly significant refactoring. Most importantly, it combines the internal graph package and the top-level dig package into a single package. Since `Container` and `Graph` are basically the same thing, this simplifies the code a fair bit without expanding the exported API at all. More importantly, it lets us *improve* overall code coverage with *just* the end-to-end tests: ``` $ go test -cover . ok go.uber.org/dig 0.010s coverage: 91.1% of statements ``` This gives us the freedom to refactor and add features aggressively, since we're only testing important user-facing APIs. Features removed (at least for now): - `Container.Resolve`: Fx doesn't need it and we don't want users interacting with dig directly, so it can go. - `Container.Invoke`: now only runs the supplied function. It *doesn't* provide any dependencies. (We can always add that back if necessary, but I like the clean separation between providing deps and triggering side effects.) Features added: - Consistent support for scalars, structs, functions (with caveats noted in the tests), and channels. All the end-to-end tests pass now.

The refactor and Params made it quite trivial to add support and e2e test for optional tags. Reading the test explains the expected behavior quite well. Stacked on #70, #72, #71

* Add golint overcommit * Add check_lint file * Lint before commit * Change the hook to be PrePush * Add missing newline

akshayjshah requested review from abhinav, glibsm, HelloGrayson, nomis52 and anuptalwalkar May 26, 2017 22:29

akshayjshah mentioned this pull request May 26, 2017

[2/2] Fix end-to-end behavior tests #72

Merged

HelloGrayson approved these changes May 27, 2017

View reviewed changes

ghost reviewed May 30, 2017

View reviewed changes

Use pointers in e2e tests

ec46812

prashantv reviewed May 30, 2017

View reviewed changes

glibsm mentioned this pull request May 31, 2017

Add support for optional parameters #77

Merged

abhinav added 7 commits May 31, 2017 10:26

pointer*: assert value

850b9e0

struct*: redundant reference

a9fc2ca

slice*: verify that values match

88f3cd0

nil test: skip instead of fatal

a5a8e37

cache: assert constructor calls counts

6899b6e

known type fails: more comprehensive

aee8ee9

test for providing a type that implements error

849e746

abhinav approved these changes May 31, 2017

View reviewed changes

prashantv reviewed May 31, 2017

View reviewed changes

prashantv approved these changes May 31, 2017

View reviewed changes

super nit

e3bfd0c

glibsm approved these changes May 31, 2017

View reviewed changes

abhinav changed the base branch from master to dev June 1, 2017 19:02

abhinav merged commit ec2f2f3 into dev Jun 1, 2017

abhinav deleted the ajs-add-behavior-tests branch June 1, 2017 19:14

hbdf pushed a commit to hbdf/dig that referenced this pull request Jul 14, 2022

Lint before pushing branches to repo (uber-go#70)

480d204

* Add golint overcommit * Add check_lint file * Lint before commit * Change the hook to be PrePush * Add missing newline

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1/2] Add tests for user-facing behavior #70

[1/2] Add tests for user-facing behavior #70

akshayjshah commented May 26, 2017 •

edited

HelloGrayson left a comment

ghost May 30, 2017

akshayjshah May 30, 2017

ghost May 30, 2017

akshayjshah May 30, 2017

ghost May 30, 2017

akshayjshah May 30, 2017

ghost May 30, 2017

akshayjshah May 30, 2017

prashantv left a comment

prashantv May 30, 2017

akshayjshah May 31, 2017

prashantv May 30, 2017

akshayjshah May 31, 2017

prashantv May 30, 2017

prashantv May 30, 2017

akshayjshah May 31, 2017

prashantv May 30, 2017

akshayjshah May 31, 2017

prashantv May 30, 2017

prashantv May 30, 2017

abhinav May 31, 2017

prashantv May 30, 2017

prashantv May 30, 2017

prashantv May 31, 2017

abhinav May 31, 2017

[1/2] Add tests for user-facing behavior #70

[1/2] Add tests for user-facing behavior #70

Conversation

akshayjshah commented May 26, 2017 • edited

☮️ 🙏 😇 🙇 I come in peace 🙇😇 🙏 ☮️

HelloGrayson left a 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

prashantv left a 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

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

akshayjshah commented May 26, 2017 •

edited