Skip to content

Improve integration tests#888

Merged
Avery-Dunn merged 2 commits intodevfrom
avdunn/test-fixes
Dec 17, 2024
Merged

Improve integration tests#888
Avery-Dunn merged 2 commits intodevfrom
avdunn/test-fixes

Conversation

@Avery-Dunn
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
Contributor 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
Copy Markdown
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
Copy Markdown
Contributor 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
@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