-
Notifications
You must be signed in to change notification settings - Fork 148
Improve integration tests #888
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
static void assertTokenResultNotNull(IAuthenticationResult result, boolean checkAccessToken, boolean checkIDToken) { | ||
assertNotNull(result); | ||
if (checkAccessToken) assertNotNull(result.accessToken()); | ||
if (checkIDToken) assertNotNull(result.idToken()); |
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.
You can add a few more asserts here. If the Id token is not null, the Account should also not be null.
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 was just meant to replace the "result & tokens aren't null" check the happens in a lot of tests and I wanted to keep the changes limited on the first PR. The tests that care about the account still have their own asserts, and the next time I work with those tests I'll add another helper method for checking various account info.
|
||
static void assertTokenResultNotNull(IAuthenticationResult result, boolean checkAccessToken, boolean checkIDToken) { | ||
assertNotNull(result); | ||
if (checkAccessToken) assertNotNull(result.accessToken()); |
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 am not aware of any case where the access token is null?
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.
Me neither, I just added it because there were a few tests where we just checked if the result or the ID token was null but not the access token.
However, I just used a 'true' value for both token options in all the tests that now use this, so in the latest commit I changed this helper method to be assertAccessAndIdTokensNotNull
and it will always check both tokens.
Contains a number of fixes and improvements to the integration tests: