Skip to content

Migrate integration tests for spring modules#11796

Merged
saragluna merged 30 commits intoAzure:masterfrom
yiliuTo:feature/migrate-multiKeyvaultIT
Jun 13, 2020
Merged

Migrate integration tests for spring modules#11796
saragluna merged 30 commits intoAzure:masterfrom
yiliuTo:feature/migrate-multiKeyvaultIT

Conversation

@yiliuTo
Copy link
Member

@yiliuTo yiliuTo commented Jun 5, 2020

Migrate integration tests into SDK repo, IT modules include:

  • azure-spring-boot-test-core (common code shared by integration tests)
  • azure-spring-boot-test-keyvault (integration tests for key vault starter)
  • azure-spring-boot-test-aad (integration tests for aad starter)
  • azure-spring-boot-test-cosmosdb (integration tests for cosmos starter)
  • azure-spring-boot-test-application (an application used by other integration tests)

Copy link
Contributor

@JimSuplizio JimSuplizio left a comment

Choose a reason for hiding this comment

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

@jialindai @yiliuTo
I'm definitely happy to see tests being put into place. One thing though, this can't actually be checked in until the other pending PR is checked in. That PR puts the azure-spring-boot-bom into sdk/boms where it belongs. When that PR is checked in this will have to be rebased as there are directly conflicting changes.

@yiliuTo yiliuTo force-pushed the feature/migrate-multiKeyvaultIT branch 3 times, most recently from 85f6c85 to bf2bb88 Compare June 8, 2020 04:16
@yiliuTo
Copy link
Member Author

yiliuTo commented Jun 8, 2020

Hi @JimSuplizio , the integration tests(ITs) will run when CI pipeline runs because they depend on our starters. However, these ITs cannot pass until some specific resources and environment variables are built and set. We prepared test-resources.json to create required resources for each IT on the test pipeline. But what can we do to avoid ITs running on the CI pipeline?

@mitchdenny
Copy link
Contributor

We prepared test-resources.json to create required resources for each IT on the test pipeline. But what can we do to avoid ITs running on the CI pipeline?

The approach we take in the rest of the repo, and I think it would be good for Azure Spring to move this direction is make use of our test recorder/playback infrastructure. This way we store recorded HTTP responses from the underlying services and play them back. So your integration tests run as part of the normal CI runs but are running off pre-recorded responses.

I'm not sure if we have a mechanism in the repo at the moment where we exclude certain tests based on whether it is a live tests or not.

@yiliuTo
Copy link
Member Author

yiliuTo commented Jun 9, 2020

Thanks @mitchdenny . But I am not sure whether playback can meet all the requirements of our usage of test resources for we don't only need to create them, but also to control. For the key vault integration tests, we need to create an app service first and then deploy jar, restart the app service and visit the endpoint. Could the test playback infrastructure help with all the above behavior?

@mitchdenny
Copy link
Contributor

mitchdenny commented Jun 9, 2020 via email

@hemanttanwar
Copy link
Contributor

hemanttanwar commented Jun 9, 2020

Sounds like we need some way to block the test execution then in CI

I create this issue to discuss in detail and document requirement : #12000

@yiliuTo yiliuTo force-pushed the feature/migrate-multiKeyvaultIT branch from 7d64587 to 6ef4bb9 Compare June 10, 2020 06:43
@yiliuTo yiliuTo closed this Jun 10, 2020
@yiliuTo yiliuTo reopened this Jun 10, 2020
@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Jun 11, 2020

You might want to update code owners to include spring dev teams.
https://github.com/Azure/azure-sdk-for-java/blob/master/.github/CODEOWNERS

Currently mgmt team is involved in this PR because of /sdk/**/mgmt*/ matching a few files in this PR.

@saragluna
Copy link
Member

@weidongxu-microsoft Thanks for the reminding, we'll update that.

Copy link
Contributor

@JimSuplizio JimSuplizio left a comment

Choose a reason for hiding this comment

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

@yiliuTo @saragluna @hemanttanwar
I'm approving this from an Engineering Systems perspective only. There are questions around the test matrix in tests.yml concerning the platforms, java version and AZURE_TEST_HTTP_CLIENTS that need to get answered or agreement from the Java team. If this is getting checked in as-is with updates to be done later (like expanding the test matrix) then I want work items filed for each please.

MaxParallel: 2
ServiceDirectory: spring
Matrix:
Linux - Java 8 (AzureCloud):
Copy link
Contributor

Choose a reason for hiding this comment

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

@hemanttanwar @saragluna
The Java team needs to comment on this but I think the test matrix needs to be more than just linux and Java 8. Similarly, should this be testing with more than just AZURE_TEST_HTTP_CLIENTS: netty?

Copy link
Member

Choose a reason for hiding this comment

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

@JimSuplizio Yes, the matrix needs to be more than just Linux - Java 8 (just using only Linux - Java 8 to save testing time). I will remove the matrix section in tests.yml in this PR to use the default matrix defined in the test template.

SubscriptionConfiguration: $(sub-config-azure-cloud-test-resources)
AZURE_TEST_HTTP_CLIENTS: netty
TestStepMavenInputs:
options: '-Dmaven.wagon.http.pool=false $(DefaultOptions) -Dmaven.javadoc.skip=true -Drevapi.skip=true -DskipSpringITs=false -pl $(ProjectList)'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy to see this PR was updated to pass skipSpringITs into Maven instead of using an environment variable.

@saragluna
Copy link
Member

/azp run java - aggregate-reports

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@saragluna
Copy link
Member

/azp run java - aggregate-reports

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

groupId: com.microsoft.azure
safeName: azurespringboottestapplication
EnvVars:
AZURE_TEST_MODE: LIVE
Copy link
Member

Choose a reason for hiding this comment

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

I'm seeing failures here that look like they're related to the right environment variables for an AAD app not being present. If you're using the default azure credentials then setting this in the EnvVars section might fix it:

        ARM_CLIENTID: $(aad-azure-sdk-test-client-id)
        ARM_CLIENTKEY: $(aad-azure-sdk-test-client-secret)
        AZURE_TENANT_ID: $(aad-azure-sdk-test-tenant-id)

Copy link
Member

Choose a reason for hiding this comment

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

@danieljurek We're using another Azure AD (which is created by ourselves) so we'll need to add such environment variables for the pipeline. Should we create a variable group for these variables or is there somewhere we could add them to? Seems like I don't have the permission to edit the pipeline.

- template: ../../eng/pipelines/templates/jobs/archetype-sdk-tests.yml
parameters:
TimeoutInMinutes: 240
MaxParallel: 2
Copy link
Member

Choose a reason for hiding this comment

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

MaxParallel is probably not necessary here. For the most part you probably only need to set it when one or more of these is true:

  • Tests use some shared resource (you're using test-resources.json which gives you one set of resources per job)
  • The resources you're creating have a low upper bound for the number of instances which can be created in a subscription and you want to limit parallelism to prevent having too many instances of that resource active at once
  • Your test matrix is enormous and running tests would saturate our agent pool and make other tests wait a long time in a queued state

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I'll remove it then.

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.

7 participants