[JDK-Matrix] Testing JDK HttpClient Matrix in AppConfig#35387
Conversation
mssfang
changed the title
|
API change check API changes are not detected in this pull request. |
|
/azp run java - appconfiguration - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| <maven.compiler.source>17</maven.compiler.source> | ||
| <maven.compiler.target>17</maven.compiler.target> |
There was a problem hiding this comment.
We should leave the compiler source and target as Java 11
| * Environment values currently supported are: "ALL", "jdk", "netty", "okhttp" which is case insensitive. | ||
| * Use comma to separate http clients want to test. | ||
| * e.g. {@code set AZURE_TEST_HTTP_CLIENTS = NettyAsyncHttpClient, OkHttpAsyncHttpClient} | ||
| * e.g. {@code set AZURE_TEST_HTTP_CLIENTS = JdkHttpClient, NettyAsyncHttpClient, OkHttpAsyncHttpClient} |
There was a problem hiding this comment.
There needs to be further updates here to actually support JDK HttpClient. The original design isn't very flexible and requires a lot of updating to support different HttpClients.
Let's have a design overhaul here and have AZURE_TEST_HTTP_CLIENTS in the matrix pass the fully qualified HttpProvider class name and then leverage HttpClientOptions.setHttpClientProvider to configure the HttpClient used in testing. This way, if/when we add support for more HttpClients in the future the only change required is adding it as a dependency in azure-core-test and everything will just work. For now, we'll still want to support netty and okhttp as options as removing this would require all SDKs to use from source azure-core-test which we don't want for now.
Additional note, not all Storage SDKs use azure-core-test's logic here for determining which HttpClient to use, we'll need to work with them to update their test code while they transition over to Test Proxy and JUnit instead of local playback tests and Spock.
| "Agent": { | ||
| "ubuntu-20.04": { "OSVmImage": "MMSUbuntu20.04", "Pool": "azsdk-pool-mms-ubuntu-2004-general" } | ||
| }, | ||
| "JavaTestVersion": "1.17", | ||
| "AZURE_TEST_HTTP_CLIENTS": "jdk", | ||
| "TestFromSource": false, | ||
| "RunAggregateReports": false, | ||
| "TestGoals": "surefire:test", | ||
| "TestOptions": "" |
There was a problem hiding this comment.
For now, let's leave this change out and just add it into the matrix AppConfiguration is using, this way if the change has issues we don't break all CI runs and only AppConfiguration which we can react to quickly.
|
Remember, to validate this change live testing must pass as these configurations don't get used by CI testing, or at least the |
|
Hi @mssfang. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
github-actions
Bot
added
the
no-recent-activity
|
Hi @mssfang. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing |
3 participants
A follow up and split PR from #34649
This PR will test JDK HttpClient Matrix Only.