Improve Netty test variants #1721
Conversation
* Only run driver-core and driver-reactive-streams test suites, each in its own task (as we do in other variants). * Use Netty configuration in all unified reactive tests, increasing coverage of Netty in driver-reactive-streams JAVA-5887
There was a problem hiding this comment.
Pull Request Overview
This PR improves the Netty test variants by ensuring that unified reactive tests use the Netty configuration for increased coverage and by streamlining async transport settings across test suites. Key changes include:
- Modifying UnifiedReactiveStreamsTest to override MongoClientSettings with custom TransportSettings.
- Removing duplicate client creation logic from ClientSideOperationTimeoutTest.
- Updating ClusterFixture and Evergreen configuration to use the new ASYNC_TRANSPORT property.
- Adjusting .evergreen run-tests scripts and matrix configurations accordingly.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/unified/UnifiedReactiveStreamsTest.java | Applies overridden TransportSettings when present. |
| driver-reactive-streams/src/test/functional/com/mongodb/reactivestreams/client/unified/ClientSideOperationTimeoutTest.java | Removes the custom createMongoClient override. |
| driver-core/src/test/functional/com/mongodb/ClusterFixture.java | Updates property retrieval to use "org.mongodb.test.async.transport". |
| .evergreen/run-tests.sh | Adjusts environment variable usage from STREAM_TYPE to ASYNC_TRANSPORT. |
| .evergreen/.evg.yml | Updates matrix specs and tasks to reflect the new async transport settings. |
Comments suppressed due to low confidence (1)
driver-core/src/test/functional/com/mongodb/ClusterFixture.java:523
- [nitpick] Consider renaming 'asyncTransport' to 'asyncTransportType' to clarify that the variable represents a transport type configuration.
String asyncTransport = System.getProperty("org.mongodb.test.async.transport", "nio2");
| @@ -291,6 +291,7 @@ functions: | |||
| export AWS_TEMP_SESSION_TOKEN=$CSFLE_AWS_TEMP_SESSION_TOKEN | |||
| export CRYPT_SHARED_LIB_PATH=${CRYPT_SHARED_LIB_PATH} | |||
|
|
|||
| ASYNC_TRANSPORT="${ASYNC_TRANSPORT}" NETTY_SSL_PROVIDER="${NETTY_SSL_PROVIDER}" \ | |||
There was a problem hiding this comment.
Add support for the Netty variants in the main "run tests" function and remove the "run netty tests" function below.
| @@ -1835,6 +1819,13 @@ axes: | |||
| display_name: NoSSL | |||
| variables: | |||
| SSL: "nossl" | |||
| - id: async_transport | |||
There was a problem hiding this comment.
This is the only way I could figure out how to set a variable. It's not possible afaik to set it directly on the matrix definition.
| display_name: "Netty SSL provider: ${version} ${topology} ${ssl} SslProvider.${netty-ssl-provider} ${auth} ${jdk} ${os} " | ||
| tags: ["tests-netty-variant"] | ||
| tasks: | ||
| - name: "netty-test" | ||
| - name: "test-reactive" |
There was a problem hiding this comment.
Spilt into the two tasks to make it run faster, as has already been done for the main test matrix. Core and reactive are the only two modules that pay attention to the transport settings.
| # MONGODB_URI Set the suggested connection MONGODB_URI (including credentials and topology info) | ||
| # TOPOLOGY Allows you to modify variables and the MONGODB_URI based on test topology | ||
| # Supported values: "server", "replica_set", "sharded_cluster" | ||
| # COMPRESSOR Set to enable compression. Values are "snappy" and "zlib" (default is no compression) | ||
| # STREAM_TYPE Set the stream type. Values are "nio2" or "netty". Defaults to "nio2". | ||
| # ASYNC_TRANSPORT Set the async transport. Values are "nio2" or "netty". |
There was a problem hiding this comment.
Changed the name to match the new TransportSettings class introduced in 5.0
| @@ -34,13 +34,14 @@ SSL=${SSL:-nossl} | |||
| MONGODB_URI=${MONGODB_URI:-} | |||
| TOPOLOGY=${TOPOLOGY:-server} | |||
| COMPRESSOR=${COMPRESSOR:-} | |||
| STREAM_TYPE=${STREAM_TYPE:-nio2} | |||
There was a problem hiding this comment.
No need to default it since it's already defaulted in ClusterFixture
| TESTS=${TESTS:-test} | ||
| SLOW_TESTS_ONLY=${SLOW_TESTS_ONLY:-false} | ||
|
|
||
| export ASYNC_TYPE="-Dorg.mongodb.test.async.type=${STREAM_TYPE}" | ||
| if [ "${ASYNC_TRANSPORT}" != "" ]; then | ||
| readonly JAVA_SYSPROP_ASYNC_TRANSPORT="-Dorg.mongodb.test.async.transport=${ASYNC_TRANSPORT}" |
There was a problem hiding this comment.
match the style just below for JAVA_SYSPROP_NETTY_SSL_PROVIDER
| @@ -138,13 +132,6 @@ public void shouldPassAllOutcomes( | |||
| } | |||
| assertNoDroppedError(format("%s passed but there was a dropped error; `onError` called with no handler.", testDescription)); | |||
| } | |||
| @Override | |||
There was a problem hiding this comment.
Push this down to the base class for all reactive tests.
| @@ -1835,6 +1819,13 @@ axes: | |||
| display_name: NoSSL | |||
| variables: | |||
| SSL: "nossl" | |||
| - id: async_transport | |||
|
@rozza do you want me to hold off on merging this? |
JAVA-5887