wip: example of using gotest.tools

[vdemeester]

Using fs, assert and env. This also enhance the tests as it
removes the temporary folder after test execution and same for
environment variables.

This is more an experimentation and how knative maintainers would feel about using gotest.tools for writing tests.

The diff seems big because of the vendoring bump 😅 👼

Signed-off-by: Vincent Demeester vdemeest@redhat.com

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vdemeester
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: shashwathi

If they are not already assigned, you can assign the PR to them by writing /assign @shashwathi in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vdemeester]

/cc @bobcatfish @shashwathi

[bobcatfish]

I think I am into this! I'm curious to hear what @shashwathi and @pivotal-nader-ziada 's thoughts are as well.

I mostly think this is pretty readable - as long as we make an effort to add error messages to our assertions so we don't lose context

I'm curious @vdemeester if you have any thoughts about this particular issues with assertion libs, specifically that we potentially lose some type safety? might be worth it anyway

Either way I think the fs lib is super useful at least

[bobcatfish]

oh nice, this fs library is nifty!!

[bobcatfish]

we should probably execute dir.Remove on interrupt as well? (same for other defer uses in this PR)

[bobcatfish]

my only request for uses of this assert lib is that we include an error message as well 🙏

as this line currently is, i think we're going to end up with an error something like "expected result to be nil but was docker credential write error" and i think it just gets really hard to understand context with just that

so maybe something lijke

assert.NilError(t, NewBuilder().Write(), "trying to write docker credentials to $HOME")

(quick acknowledgement that the original error message here was pretty lacking 😅 )

[nader-ziada]

My concern is with using a not very common library I guess, just makes the project depend on something that most people are not familiar with and that might not be maintained consistently in the future

[shashwathi]

My 2 cents on this PR.

Why I would consider this change?

• Less verbosity in test. We can achieve this by adding test helpers like testSetup(), testTearDown() functions to break down long test instead of adding new library for this purpose.

Why I would not consider this change?

• Looks like there was an attempt to try diff test framework and there is good feedback on why we should / should not go down this path again. Ref: Evaluate test frameworks knative/serving#253
• For a developer who is contributing code to any knative repo, it should be reasonable to expect code structure and testing to be homogenous. I think embracing this lib will take us away from that. So if this is something we want to embark on then I would propose submitting to Productivity WG and making this common across all repos.
• There is no extra information provided by this dependency for debugging test failures than current testing framework so as a developer I am not seeing any benefits.

[bobcatfish]

I think @shashwathi and @pivotal-nader-ziada have some good points!

The one point though that I'm not quite in agreement with is:

For a developer who is contributing code to any knative repo, it should be reasonable to expect code structure and testing to be homogenous. I think embracing this lib will take us away from that. So if this is something we want to embark on then I would propose submitting to Productivity WG and making this common across all repos.

I think deciding that everything has to be the same across all repos for knative projects will make it extremely hard to try new things; I'd prefer an approach where we can experiment with trying new things and see how it goes (and from there we can propose adopting new libs etc. across all repos). Plus once we start tackling #267 we're probably gonna be in a very different place from the rest of the knative repos (tho I suppose they may eventually move to using pipelines as well!).

I think the other points are good ones tho! So at the moment it seems like the majority opinion is to not move to using these helpers :S

@shashwathi @pivotal-nader-ziada what do you think about using just the fs lib for now? Seems pretty handy?

[shashwathi]

what do you think about using just the fs lib for now? Seems pretty handy?

Sure 👍

I tried to use combination of fs(create files) and cmp (to compare contents). I think to really reap benefits its either fs library all the way or no way.

expectedConfig := `{"auths":{"https://us.gcr.io":{"username":"bar","password":"baz","auth":"YmFyOmJheg==","email":"not@val.id"}}}`
	expected := fs.Expected(t, fs.WithDir(".docker",
		fs.WithFile("config.json", expectedConfig, fs.MatchAnyFileMode),
	), fs.MatchExtraFiles)

	if d := cmp.Diff(expected, credentials.VolumePath); d != "" {
		t.Errorf("life is partially awesome: %s", d)
	}

It doesn't work. If there is good intermediate ground I am onboard

@bobcatfish

[bobcatfish]

It doesn't work. If there is good intermediate ground I am onboard

aw shucks :s

Well let's see if @vdemeester has any thoughts on what we've discussed so far when he's back online

[bobcatfish]

A couple more thoughts:

Looks like there was an attempt to try diff test framework and there is good feedback on why we should / should not go down this path again. Ref: knative/serving#253

I am wholeheartedly against test (or any) frameworks for sure, but I think we could use a lib for assertions without going all the way down the framework path.

My concern is with using a not very common library I guess, just makes the project depend on something that most people are not familiar with

I guess this could apply to the builders too but I think those are making our tests way easier to deal with, so I think it's worth the slight extra hurdle.

I think once you start to deviate from what one expects as "go style" it becomes a problem - my naive pov is that the builders are pretty "golang-esque", maybe we don't feel like the assertion helpers are.

and that might not be maintained consistently in the future

Maybe @vdemeester can shed more light on this?

[nader-ziada]

I feel like my concern applies to using the library (or part of it), so the same applies to fs, but if everybody else wants it, then it's fine.

On a side note; I personally find the builders harder to read when you are debugging a test that you didn't write yourself, I understand it makes writing the test easier, but code readability is important as well.

[vdemeester]

@pivotal-nader-ziada @shashwathi @bobcatfish thanks a lot for the feedback 😻

My concern is with using a not very common library I guess, just makes the project depend on something that most people are not familiar with and that might not be maintained consistently in the future

@pivotal-nader-ziada Right, I hear you and understand the fear 👼. I feel it's not really a big deal for several reason:

• we vendor dependencies, if for some reason it goes away, it's easy to "fork" it and maintain it
• it would be pretty easy adding some knative code owner as maintainer of the project too (to make sure it's maintain)

… but that's a risk indeed 😅 .

For a developer who is contributing code to any knative repo, it should be reasonable to expect code structure and testing to be homogenous. I think embracing this lib will take us away from that. So if this is something we want to embark on then I would propose submitting to Productivity WG and making this common across all repos.

@shashwathi As @bobcatfish I completely agree with that, I openened the PR as a "let's gather some initial feedback first on a small group" 😉. If we adopt a testing helper library in the future (this one, another, …), it should to be done across all knative projects for sure 👼

There is no extra information provided by this dependency for debugging test failures than current testing framework so as a developer I am not seeing any benefits.

So, you could see gotest.tools assertion part (assert package) as a nice addition on top of github.com/google/go-cmp (which is used internally). This means when you do an assert.Assert(fs.Equal(…), "a message") it will not only print the message, but a consistant error message — more or less the same that we do manually on our test today (something -want, +got: %s)

I tried to use combination of fs(create files) and cmp (to compare contents). I think to really reap benefits its either fs library all the way or no way.

[…]

It doesn't work. If there is good intermediate ground I am onboard

@shashwathi @bobcatfish Sadly yes it doesn't work as the fs.Equal uses gotest.tools own Comparison type that builds on top of go-cmp (see https://github.com/gotestyourself/gotest.tools/blob/master/fs/report.go#L23). We could make it work by exporting eqDirectory (and refactor it a tad) in gotest.tools so that you could use it outside of the assert part of gotest.tools. Or use only the fs "creation" part of it (aka fs.NewDir(t, …)), but we would be better just "mimicing" it in an helper in knative project(s).

I am wholeheartedly against test (or any) frameworks for sure, but I think we could use a lib for assertions without going all the way down the framework path.

I'm with you on that. gotest.tools is definitely not a test framework. It's an testing helper library that builds on top of google/go-cmp — built to work with testing.T or others (like go-check) and with go support from 1.7 to now (more or less 👼)

On a side note; I personally find the builders harder to read when you are debugging a test that you didn't write yourself, I understand it makes writing the test easier, but code readability is important as well.

@pivotal-nader-ziada I don't agree too much on the "readability" 😅 (but I may be wrong 🙃 ). The goal of those builder is readability and less noise : have in tests the relevant information required by tests and as less as possible else. It's meant for the reader to quickly see what matters for the test (and what would not). It makes writing the test easier but the main goal is to make it readable too – as less "reading travel" between the object definition and it's usage in test.
I'm hoping it to be more readable for others 👼 If it's not, then I have more work to do on them 👼 🙃 😛

[shashwathi]

we vendor dependencies, if for some reason it goes away, it's easy to "fork" it and maintain it

Been there, done that and I personally don't wanna go down that path again. Well maintained library, actively in development, more adopted are good signs.

So, you could see gotest.tools assertion part (assert package) as a nice addition on top of github.com/google/go-cmp (which is used internally). This means when you do an assert.Assert(fs.Equal(…), "a message") it will not only print the message, but a consistant error message — more or less the same that we do manually on our test today (something -want, +got: %s)

To demonstrate that with a test failure with assert lib

 content:
		    --- expected
		    +++ actual
		    @@ -1 +1 @@
- {...}
+ {...}

Note : expected and actual are part of assert lib output.

with just go-cmp and not assert

/Users/pivotal/workspace/golang/src/github.com/knative/build-pipeline/pkg/credentials/dockercreds/creds_test.go:116: -want +got diff {string}:
- {...}
+ {...}

Test error msg does not contain (-want, +got) and developers need to add this to error msg. Am I stating that correctly @vdemeester ?

Is the fs lib providing more information to debug that go-cmp is not providing?
No in my opinion.
If fs is built on top of go-cmp then the value add is editing the output to be more pretty. Personally I am not convinced about the benefit.

[vdemeester]

Been there, done that and I personally don't wanna go down that path again. Well maintained library, actively in development, more adopted are good signs.

Fair enough 😉

To demonstrate that with a test failure with assert lib

 content:
		    --- expected
		    +++ actual
		    @@ -1 +1 @@
- {...}
+ {...}

Note : expected and actual are part of assert lib output.

with just go-cmp and not assert

/Users/pivotal/workspace/golang/src/github.com/knative/build-pipeline/pkg/credentials/dockercreds/creds_test.go:116: -want +got diff {string}:
- {...}
+ {...}

Test error msg does not contain (-want, +got) and developers need to add this to error msg. Am I stating that correctly @vdemeester ?

Right it's a bit more "sugary" than that. The content here comes from the fs.Equal comparator. It would show sthg like the following (if error).

 assertion failed: directory /tmp/-709862210 does not match expected:
        /.docker/config.json
          content:
            --- expected
            +++ actual
            @@ -1 +1 @@
            -{"auth":{"https://us.gcr.io":{"username":"bar","password":"baz","auth":"YmFyOmJheg==","email":"not@val.id"}}}
            +{"auths":{"https://us.gcr.io":{"username":"bar","password":"baz","auth":"YmFyOmJheg==","email":"not@val.id"}}}

If multiple files differ from expected to actual, it would display them (newline, with /another/file for example).

The assertion library does also more than just displaying expected/actual. It will take the variables names (for DeepEqual or Equal for ex) in the failure output.

assert.Equal(t, foo, bar)
[…]
--- foo
+++ bar
@@ -1,4 +1,4 @@
 abcd
-1234
+1111
 aaaa
 bbbb

Other helper get other message output

assert.Error(t, …)
[…]
expected error "the error message", got "wrapped: other"
other
gotest.tools/assert/cmp.TestError

assert.ErrorContains(t, …)
[…]
expected error to contain "the error", got "other"

Is the fs lib providing more information to debug that go-cmp is not providing?
No in my opinion.
If fs is built on top of go-cmp then the value add is editing the output to be more pretty. Personally I am not convinced about the benefit.

Fair enough 👼 we can achieve what's done in this PR with small local helper too 😉

As this PR was mainly about getting feedback, I'm gonna go ahead and close it, thanks for the feedback 👼 👍

[bobcatfish]

Thanks for fostering the discussion @vdemeester , hopefully we can revisit this later on, I think there's some really cool stuff in the lib! ❤️