Skip to content
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

chore(azure/build): improve test times for azure #6246

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

richard-timpson
Copy link
Contributor

@richard-timpson richard-timpson commented Jul 9, 2024

Some azure tests need the AzureCredentials class. These tests create the class by using the default AzureCredentials constructor. The constructor for the class has code that tries to actually authenticate against a Microsoft/Azure server, and all of the API calls to the MS servers fail. The tests all have code to set up in this class in the setup section of Groovy specs, which means that gradle never actually executes the tests for these classes. The API calls to MS fail and are repeatedly retried over and over again until they eventually stop and the tests run, and succeed. I'm not sure why the tests end up passing after the retries are exhausted.

This issue can only be found when running a command like ./gradlew :clouddriver-azure:test --debug which shows the logs of the gradle runner attempting to execute the test. When running that command, errors like

com.microsoft.aad.msal4j.MsalServiceException: AADSTS900023: Specified tenant identifier 'azureclientid' is neither a valid DNS name, nor a valid external domain. Trace ID: cecb379b-a3df-408d-aae5-7c7d62bd2300 Correlation ID: b646b2a1-4371-4720-

frequently appear. The tests also take between 7-10 minutes to run.

With this change because the AzureCredentials constructor is no longer invoked and only mocked, the MS authentication API is never hit, and the tests complete immediately, without any API failures.

This should significantly improve the build times of clouddriver both locally and in CI environments.

After applying the change and running ./gradlew clean :clouddriver-azure:test the tests complete in a few seconds, instead of minutes.

Some azure tests need the AzureCredentials class. These tests create the class by using the default AzureCredentials constructor. The constructor for the class has code that tries to actually authenticate against a Microsoft/Azure server, and all of the API calls to the MS servers fail. The tests all have code to set up in this class in the setup section of Groovy specs, which means that gradle never actually executes the tests for these classes. The API calls to MS fail and are repeatedly retried over and over again until they eventually stop and the tests run, and succeed. I'm not sure why the tests end up passing after the retries are exhausted.

This issue can only be found when running a command like `./gradlew :clouddriver-azure:test --debug` which shows the logs of the gradle runner attempting to execute the test. When running that command, errors like

```
com.microsoft.aad.msal4j.MsalServiceException: AADSTS900023: Specified tenant identifier 'azureclientid' is neither a valid DNS name, nor a valid external domain. Trace ID: cecb379b-a3df-408d-aae5-7c7d62bd2300 Correlation ID: b646b2a1-4371-4720-
```

frequently appear. The tests also take between 7-10 minutes to run.

With this change because the AzureCredentials constructor is no longer invoked and only mocked, the MS authentication API is never hit, and the tests complete immediately, without any API failures.

This should significantly improve the build times of clouddriver both locally and in CI environments.
@dbyron-sf dbyron-sf added the ready to merge Approved and ready for a merge label Jul 9, 2024
@mergify mergify bot added the auto merged Merged automatically by a bot label Jul 9, 2024
@mergify mergify bot merged commit dc19c3c into spinnaker:master Jul 9, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged Merged automatically by a bot ready to merge Approved and ready for a merge target-release/1.35
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants