Skip to content

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

Merged
merged 2 commits into from
Dec 17, 2024
Merged

Improve integration tests #888

merged 2 commits into from
Dec 17, 2024

Conversation

Avery-Dunn
Copy link
Collaborator

@Avery-Dunn Avery-Dunn commented Dec 17, 2024

Contains a number of fixes and improvements to the integration tests:

  • Use 'common' authority instead of 'organizations' for ADFS tests
    • Fixes an issue caused by a change to a test app, and is also what is used in other MSALs
  • Update queries to use newer labs resources
  • Remove some unused code/constants/imports/etc., and refactor some common code into a new IntegrationTestHelper class
    • Just added some low-hanging fruit for now, more refactoring to come in future PRs when it makes sense

@Avery-Dunn Avery-Dunn requested a review from a team as a code owner December 17, 2024 19:14
static void assertTokenResultNotNull(IAuthenticationResult result, boolean checkAccessToken, boolean checkIDToken) {
assertNotNull(result);
if (checkAccessToken) assertNotNull(result.accessToken());
if (checkIDToken) assertNotNull(result.idToken());
Copy link
Member

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.

Copy link
Collaborator Author

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());
Copy link
Member

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?

Copy link
Collaborator Author

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.

@Avery-Dunn Avery-Dunn merged commit 95b5efc into dev Dec 17, 2024
5 checks passed
@Avery-Dunn Avery-Dunn deleted the avdunn/test-fixes branch December 17, 2024 22:47
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.

2 participants