Skip to content

[JDK-Matrix] Testing JDK HttpClient Matrix in AppConfig #35387

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.

Sign up for GitHub

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

Closed
mssfang wants to merge 1 commit into from
Closed

[JDK-Matrix] Testing JDK HttpClient Matrix in AppConfig #35387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions eng/pipelines/templates/stages/platform-matrix.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,17 @@
"RunAggregateReports": false,
"TestGoals": "surefire:test",
"TestOptions": ""
},
{
"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": ""
Comment on lines +59 to +67

@alzimmermsft alzimmermsft Jun 9, 2023

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.

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.

}
]
}
16 changes: 16 additions & 0 deletions sdk/appconfiguration/platform-matrix.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"matrix": {
"$IMPORT": "eng/pipelines/templates/stages/platform-matrix.json"
},
"exclude": [
{ "OSVmImage": "macos-11" }
],
"include": [
{
"Agent": { "windows-2022": { "OSVmImage": "windows-2022", "Pool": "azsdk-pool-mms-win-2022-general" } },
"JavaTestVersion": "1.17",
"AZURE_TEST_HTTP_CLIENTS": "jdk",
"TestFromSource": false
}
]
}
20 changes: 15 additions & 5 deletions sdk/core/azure-core-http-jdk-httpclient/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@

<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<maven.compiler.source>11</maven.compiler.source>
<maven.compiler.target>11</maven.compiler.target>
<maven.compiler.source>17</maven.compiler.source>
<maven.compiler.target>17</maven.compiler.target>
Comment on lines +47 to +48

@alzimmermsft alzimmermsft Jun 9, 2023

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.

We should leave the compiler source and target as Java 11

<jacoco.skip>true</jacoco.skip>
<legal>
<![CDATA[[INFO] Any downloads listed may be third party software. Microsoft grants you no rights for third party software.]]></legal>
Expand All @@ -56,9 +56,19 @@
<!-- Enables fail on deprecated API usage. -->
<compiler.failondeprecatedstatus/>

<javadoc.excludePackageNames>com.azure.json,com.azure.core.implementation*,com.azure.core.util,com.azure.core.util*,
com.azure.core.models,com.azure.core.http,com.azure.core.http.policy,com.azure.core.http.rest,com.azure.core.exception,com.azure.core.cryptography,
com.azure.core.credential,com.azure.core.client.traits,com.azure.core.annotation</javadoc.excludePackageNames>
<javadoc.excludePackageNames>
com.azure.json,com.azure.core.implementation*,
com.azure.core.util*,
com.azure.core.models,
com.azure.core.http,
com.azure.core.http.policy,
com.azure.core.http.rest,
com.azure.core.exception,
com.azure.core.cryptography,
com.azure.core.credential,
com.azure.core.client.traits,
com.azure.core.annotation
</javadoc.excludePackageNames>
</properties>

<developers>
Expand Down
4 changes: 2 additions & 2 deletions sdk/core/azure-core-test/src/main/java/com/azure/core/test/TestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,9 +277,9 @@ public static Stream<HttpClient> getHttpClients() {
* <li>Otherwise, the name of the HttpClient class should match env variable.</li>
* </ul>
*
* Environment values currently supported are: "ALL", "netty", "okhttp" which is case insensitive.
* 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}

@alzimmermsft alzimmermsft Jun 9, 2023

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.

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.

*
* @param client Http client needs to check
* @return Boolean indicates whether filters out the client or not.
Expand Down