-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
wip: example of using gotest.tools
#409
Conversation
Using `fs`, `assert` and `env`. This also enhance the tests as it removes the temporary folder after test execution and same for environment variables. Signed-off-by: Vincent Demeester <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: vdemeester If they are not already assigned, you can assign the PR to them by writing 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:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
dir := fs.NewDir(t, "", fs.WithDir("foo", | ||
fs.WithFile(corev1.BasicAuthUsernameKey, "bar"), | ||
fs.WithFile(corev1.BasicAuthPasswordKey, "baz"), | ||
)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh nice, this fs
library is nifty!!
fs.WithFile(corev1.BasicAuthUsernameKey, "bar"), | ||
fs.WithFile(corev1.BasicAuthPasswordKey, "baz"), | ||
)) | ||
defer dir.Remove() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably execute dir.Remove
on interrupt as well? (same for other defer
uses in this PR)
t.Fatalf("ioutil.ReadFile(.docker/config.json) = %v", err) | ||
} | ||
defer env.Patch(t, "HOME", credentials.VolumePath)() | ||
assert.NilError(t, NewBuilder().Write()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😅 )
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 |
My 2 cents on this PR. Why I would consider this change?
Why I would not consider this change?
|
I think @shashwathi and @pivotal-nader-ziada have some good points! The one point though that I'm not quite in agreement with is:
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 |
Sure 👍 I tried to use combination of
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 |
A couple more thoughts:
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 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.
Maybe @vdemeester can shed more light on this? |
I feel like my concern applies to using the library (or part of it), so the same applies to 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 @shashwathi @bobcatfish thanks a lot for the feedback 😻
@pivotal-nader-ziada Right, I hear you and understand the fear 👼. I feel it's not really a big deal for several reason:
… but that's a risk indeed 😅 .
@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 👼
So, you could see
@shashwathi @bobcatfish Sadly yes it doesn't work as the
I'm with you on that.
@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. |
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.
To demonstrate that with a test failure with assert lib
Note : with just go-cmp and not assert
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 |
Fair enough 😉
Right it's a bit more "sugary" than that. The
If multiple files differ from expected to actual, it would display them (newline, with The assertion library does also more than just displaying
Other helper get other message output
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 👼 👍 |
Thanks for fostering the discussion @vdemeester , hopefully we can revisit this later on, I think there's some really cool stuff in the lib! ❤️ |
Using
fs
,assert
andenv
. This also enhance the tests as itremoves 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 [email protected]