Skip to content

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

Merged
merged 2 commits into from
May 12, 2020

Conversation

hiranya911
Copy link
Contributor

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.

Copy link
Contributor

@micahstairs micahstairs left a 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();
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM!

@hiranya911 hiranya911 merged commit bfac46a into idp-config May 12, 2020
@hiranya911 hiranya911 deleted the hkj-user-cleanup branch May 12, 2020 18:37
micahstairs pushed a commit that referenced this pull request Jun 12, 2020
* fix(auth): Ensuring test user account cleanup with a Rule

* Updated copyright holder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants