-
Notifications
You must be signed in to change notification settings - Fork 269
Refactoring the user management integration tests #152
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
Conversation
} | ||
|
||
func deleteUser(uid string) { | ||
client.DeleteUser(context.Background(), uid) |
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 feel like we should at least check the error state and print out a warning or something.
I'm not sure if it would make sense, but we could maybe pass a *testing.T into this function and fail the test if the delete fails.
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 intended to be a best effort delete operation. I don't think we want to fail a test because the clean up operation failed. Added a comment stating this.
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 still think it would be good to print out a warning when it fails.
func randomString(length int) string { | ||
b := make([]byte, length) | ||
func randomUID() string { | ||
var letters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789") |
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.
Can this be const?
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.
Arrays cannot be const
in Go.
integration/auth/user_mgt_test.go
Outdated
} | ||
|
||
func randomPhoneNumber() string { | ||
var letters = []rune("0123456789") |
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 would maybe name this digits
.
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.
Done
integration/auth/auth_test.go
Outdated
} | ||
|
||
func TestCustomTokenWithClaims(t *testing.T) { | ||
ct, err := client.CustomTokenWithClaims(context.Background(), "user2", map[string]interface{}{ |
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.
Do you want to use random UIDs on all these other tests?
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.
Done
* Adding new user mgt integ tests * Reimplemented user mgt integ tests * Fixed update test * Minor updates for clarity
The existing test suite is essentially one big test case composed of many helper functions. It also keeps a lot of intermediate state (fixtures), and tends to leave the Firebase project in an inconsistent state on failures. This PR:
defer
keyword to ensure graceful tear downResolves #145