-
Notifications
You must be signed in to change notification settings - Fork 289
fix(auth): Ensuring test user account cleanup with a Rule #409
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
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.
Looks good! This is much better than try blocks.
auth = FirebaseAuth.getInstance(masterApp); | ||
} | ||
@Rule | ||
public final TemporaryUser temporaryUser = new TemporaryUser(); |
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.
Are you planning on adding a similar rule for tenants or should I take care of that?
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 might as well do it so that we don't step on each others' toes. I'm also planning on doing this for the provider configs.
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 didn't find that many instances where we create and delete provider configs. But if that's something we do repeatedly, then might be useful to make a similar change.
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.
It's not only a matter of reducing how many times we repeat the setup/cleanup. As it currently stands, if an assertion fails right after creating the provider config, it hasn't entered the "try" block yet, so the "finally" won't be executed. So we would either need to extend the try block back further, or we can use temporary provider configs. The temporary objects are nice because it is less of a burden on the developer writing integration tests, and it reduces the required nesting.
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.
Thanks! LGTM!
* fix(auth): Ensuring test user account cleanup with a Rule * Updated copyright holder
Using a JUnit
@Rule
to ensure that temporary user accounts are cleaned up in integration tests. This helps us avoid large try-finally blocks in tests.I also moved couple of new tenant management tests to a new
TenantManagerIT
class.