-
Notifications
You must be signed in to change notification settings - Fork 9
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
Extend unit tests #165
Extend unit tests #165
Conversation
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.
Amazing work!
The comments are mostly nitty, other than that looks great to me!
Side note given we are on testing: would we rename transform_test_variables.go
to transform_variables_test.go
to follow the test suffix convention?
@@ -13,6 +13,12 @@ import ( | |||
"github.com/stretchr/testify/assert" | |||
) | |||
|
|||
func TestMain(m *testing.M) { |
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.
This is great! I didn't know of TestMain!
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.
Credit goes to Robert for this one
Thanks @adatzer you've pointed out some useful things here. Going to take another pass at them tomorrow and see if I can improve this PR a bit.
I think I initially did it that way as it's not testing a file named variables, it's a set of variables for tests. But I dunno if that actually makes any sense - I'll think about it after I address the comments. |
Thanks for very useful comments @adatzer - I'm a lot happier with the state of affairs now. I think I've addressed everything now, should be ready for another pass. |
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.
This is a great PR! Incredible work!
if err == nil { | ||
t.Fatalf("expected error, got nil") | ||
} | ||
assert.NotNil(err) |
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.
nit - can we use the new if err != nil
pattern here as well?
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.
Yeah looks like I missed a few like this. Fixing
ea0c004
to
8f69ca4
Compare
This PR replaces #139, as it required a rebase and it was simple to just create a new PR.