-
Notifications
You must be signed in to change notification settings - Fork 43
Add unit tests for api-tools/utils/methods #39 #74
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
base: develop
Are you sure you want to change the base?
Add unit tests for api-tools/utils/methods #39 #74
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.
Overall looks good. I'm by all means not an expert but I commented on everything I noticed.
Also I'm not sure if I'm supposed to do reviews but I don't have write access so I guess it doesn't matter.
utils/methods_test.go
Outdated
func TestGetEnv(t *testing.T) { | ||
// Each environment variable to check for | ||
envVars := []string{ | ||
"LOGIN_NETID", "LOGIN_PASSWORD", "MONGODB_URI", "LOGIN_ASTRA_USERNAME", | ||
"LOGIN_ASTRA_PASSWORD", "MAZEVO_API_KEY", | ||
} | ||
|
||
for _, envVar := range envVars { | ||
// Get each env, and if it doesn't exist, error | ||
v, err := GetEnv(envVar) | ||
if v == "" || err != nil { | ||
t.Errorf("Environment variable %s is not set", envVar) | ||
} | ||
} | ||
} |
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 test depends on the fact that the .env loaded in the TestMain has all of these fields and that those fields are not empty. I personally only have LOGIN_NETID and LOGIN_PASSWORD set so it failed when I tested it.
I would look into using t.SetEnv(), since it doesn't matter what variables we are using to test GetEnv and this will ensure the test doesn't break if we use different env variables in the future.
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 completely agree that this test is flawed. I mainly put it in because actual login info is required for testing refresh tokens, but I completely agree that writing it this way is confusing. GetEnv is already a trivially small function that's just a wrapper around os.LookupEnv. For that reason, I feel that writing a unit test where we explicitly set the environment variables as a part of the test is basically just testing that os.LookupEnv is working correctly. The function is so small that if we ever decided to change it in the future, we would probably also have to rewrite the test. In my opinion, there is no point in testing a function this small. What do you (or anyone else) think?
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.
While I agree that the test would pretty useless, it is a test for a trivial function. My opinion is that the unit test for GetEnv
does not need to care about the greater environment as this is outside the scope of the test. The test should only ensure that the expected contract of GetEnv
is maintained (get var or error).
In regards to making sure the environment is correct, I would argue that this should be the job of TestMain
. I would prefer to see the it fail early because I am missing a variable rather than see all the tests fail (failing TestGetEnv will not stop the other tests because it uses Errorf not Fatalf).
This is just my opinion but this is probably a good time to decide how to handle the environment when testing before we start adding tests to the scraper.
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 agree with your opinion, and have moved the Environment variable testing to TestMain. I think I agree that that would probably be the best approach for other tests as well.
I appreciate the feedback! |
I will review them soon. Thanks! |
Before you review, I want to update, so im closing the PR |
I've made some minor improvements to the error messages, but I am still not sure about @SoggyRhino's main concern over environment variables in testing, especially for how other tests should handle it. For now, I have followed the advice and added this to TestMain to ensure all required variables are already set because testing these utils/methods is not possible without having the environment variables set. // The following environment variables must be set for tests to run
envVars := []string{
"LOGIN_NETID", "LOGIN_PASSWORD", "LOGIN_ASTRA_USERNAME", "LOGIN_ASTRA_PASSWORD",
}
for _, envVar := range envVars {
// Get each env, and if it doesn't exist, error
v, err := GetEnv(envVar)
if v == "" || err != nil {
log.Fatalf("Error loading environment variable %v: %v\n"+
"Valid credentials are required for testing utils/methods", envVar, err)
}
} |
Alright lemme take a look!. But first can you resolve the conflicts. |
1141189
to
c853f04
Compare
Okay, I have rebased my branch, so there are no more merge conflicts |
Ok sweet, will prioritize getting this merged on this repo. |
Fixes #39 Creates a few tests for api-tools/utils/methods
This doesn't cover all, but I'm mainly looking for feedback. There's lots of trivially small functions in api-tools/utils that I feel unit testing wouldn't really help with, but if someone has other thoughts, I would love to hear.
I also put the function name prefixes on the godoc comment because my linter wanted it, but I can easily undo it.