test(warmup): Add warm-up tests that auto-cover the generated provider of every service module - #7188
Conversation
d558d8b to
396b4c3
Compare
…iders and are executed withouut any errors
396b4c3 to
4778e7e
Compare
| * JVM with a manifest-only booter jar, so {@code java.class.path} lists only {@code surefirebooter*.jar} and scanning | ||
| * it would find nothing. | ||
| */ | ||
| class WarmUpProviderCompletenessTest { |
There was a problem hiding this comment.
Is this test really necessary? What potential issue would this test catch?
There was a problem hiding this comment.
This test catches a service jar that ships without a valid SdkWarmUpProvider registration in META-INF/services. The other two tests discover providers through ServiceLoader, and ServiceLoader only sees registered providers, so an unregistered service simply produces no test case and the suite passes while that service silently loses warm-up coverage. This test reads the classpath jars directly, so the same defect becomes a build failure.
This can happen because codegen writes the registration as a plain text file (WarmUpProviderRegistrationTask). A codegen regression can skip or corrupt that file if there is customized code for a module and without any unit test failing, and a hand-written META-INF/services file added to a module later would overwrite the generated one during packaging. This test confirms every service jar has its registration, and that is what gives the other two tests their input: they only execute providers that are registered.
There was a problem hiding this comment.
Hmm, this feels like a bit overkill IMO. I can't think of a case where we'd miss generating files for certain services by mistake. I wonder if this would introduce more work, for example, we need to update this test file whenever we need to skip certain services intentionally.
There was a problem hiding this comment.
Thanks for the feedback. Quick clarification: do you mean just the loading test (WarmUpProviderCompletenessTest), or all the tests here as well?
Their main purpose is regression prevention: making sure every module registers its META-INF service entry and that it isn't accidentally overwritten for a module later.
Happy to discuss a lighter alternative offline, and I'm fine removing it if we agree it isn't needed.
There was a problem hiding this comment.
I meant WarmUpProviderCompletenessTest (basically any tests testing implementation details instead of behavior). What's the difference between SdkWarmUpPrimeAllServicesTest and AllServicesWarmUpTest? Can we consolidate them?
There was a problem hiding this comment.
Agreed on both points.
On WarmUpProviderCompletenessTest: Removed it now
On consolidation: yes, good point, we can merge them. The two classes overlapped, they just checked different layers:
- SdkWarmUpPrimeAllServicesTest exercised the customer entry point SdkWarmUp.prime() once with every service on the classpath and asserted no provider logs a warm-up failure.
- AllServicesWarmUpTest exercised each generated provider individually and asserted the warm-up actually invoked its selected operation and emitted no SDK warnings.
I've consolidated them into a single AllServicesWarmUpTest that drives each client through the public API SdkWarmUp.prime(Client.class) (sync and async), asserting the operation is invoked with no WARN logged, and keeps one SdkWarmUp.prime() test that warms all services at once. So we now test only through public APIs.
| <description>Centralized tests for the CRaC HTTP-client warm-up, run against every sync HTTP client so each client does not | ||
| need its own warm-up test. This is a leaf test module: nothing depends on it, so depending on the HTTP clients here | ||
| does not create a dependency cycle.</description> | ||
| <description>Centralized tests for the CRaC warm-up feature. Covers the SdkWarmUp orchestrator and HTTP-client warm-up |
There was a problem hiding this comment.
nit: technically, this feature/test doesn't directly interact with CRaC, let's do a scan and remove the wording. Can be done in a separate PR
There was a problem hiding this comment.
Removed from CRaC from here will do the scan and remove remaining with another PR.
| <version>${awsjavasdk.version}</version> | ||
| <scope>test</scope> | ||
| </dependency> | ||
| <!-- Every service module, so the generated SdkWarmUpProviders of all services are on the test classpath. |
There was a problem hiding this comment.
Question: any reason we want to run tests for all services? How long does it take to run all tests?
There was a problem hiding this comment.
Yes, we run all services because the warm-up operation is selected per service from each service's model; only executing every generated provider proves each one works. The full module runs offline (Canned Req/Response).
These test guard the customer path: a customer with all services on the classpath calls SdkWarmUp.prime(), and a broken provider logs a WARN and skips that service's warm-up. These tests turn that customer-visible WARN into a build failure here.
New services are covered automatically, the aggregate dependency picks up each new service, and the completeness test fails the build if a service ships without a registered provider
There was a problem hiding this comment.
I will get the exact time once I merge the PR 7183. The issues mentioned in this PR were caught by these tests
There was a problem hiding this comment.
The building of all the modules without mvn -T1C maven option takes 50 mins.
The junits overall take 1 minute.
The overall codebuild takes 94m if executed as part of exsiting jdk Junit buildss (That was because a a failed build performed a retry)
Thus I am planning to move this as a separate test similar to v2-migration-test and s3-regression test that can run parallely.
I tested this on codebuild with the below mvn command similar to v2-migration-test
mvn clean install -pl :warmup-tests,:bom-internal -am -P quick -T1C $MAVEN_OPTIONS
mvn install -pl :warmup-tests -T2C $MAVEN_OPTIONS
This took 23.9 mins
There was a problem hiding this comment.
Apologies, the earlier 94 mins was a confusion caused by the new way the builds are handled now. There was a retry of another build (the SonarCloud step), and that retry is what showed the increase.
I pushed a dummy commit, and this time the build passed with no retries and the time is the same as before.
So to answer your question: these tests do not add time to the JDK build. The service modules are already built at that point, and those already built modules are reused by the JUnit tests, which take a minute or less.
IMO we donot need a separate test build just for this .
df31186
into
feature/master/crac_auto_priming_support
Motivation and Context
CRaC warm-up codegen emits a
<Service>WarmUpProviderfor each service module, but no test ran those generated providers. The existingwarmup-testsmodule covered theSdkWarmUporchestrator and HTTP-client warm-up with hand-written providers.This change adds tests that run the generated provider of every service. The tests put all services on the classpath through the
aws-sdk-javaaggregate artifact. The aggregate picks up each new service, so the tests cover a new service with no change to this module. A completeness test turns a new service that ships without a registered provider into a build failure instead of a silent gap.Modifications
Added to
test/warmup-tests:AllServicesWarmUpTest: parameterized over every generated provider thatServiceLoaderfinds, one sync case and one async case per provider. Each case asserts that warm-up completes without throwing, that the selected warm-up operation runs (OperationRecordingInterceptorrecords it), and that SDK loggers emit no warn or error logs (LogCaptorcaptures them, filtered to SDK loggers).KNOWN_NO_OP_PROVIDERSlists services whose selector picked no operation because every operation is streaming, event-streaming, or deprecated. The test asserts these providers record no operation, and an entry fails once codegen starts selecting an operation for that service.SdkWarmUpPrimeAllServicesTest: runs the customer entry pointSdkWarmUp.prime()with all services on the classpath and asserts it logs no warm-up failure for any generated service provider. The test scopes the assertion to service-provider warnings because the HTTP-client warm-up step logs the same message when an HTTP client fails to build.WarmUpProviderCompletenessTest: scans the classpath jars and asserts every service jar registers aSdkWarmUpProviderinMETA-INF/services. The ServiceLoader-driven tests generate no case for an unregistered service, so this test reads the jars directly. The test enumerates jars withClassLoader.getResourcesinstead ofjava.class.pathbecause surefire runs the test JVM with a manifest-only booter jar.OperationRecordingInterceptorand itsexecution.interceptorsregistration: records the name of the operation that warm-up invokes, so the tests confirm the selected operation ran.pom.xml: adds test-scope dependencies on theaws-sdk-javaaggregate (all services on the classpath),test-utils(LogCaptor), the log4j2 binding (log4j-core,log4j-slf4j-impl), andhttp-auth-aws-crt(sigv4a signer, required by services whose endpoint resolves to sigv4a). Disables JaCoCo for this module: it loads every service client, and instrumenting the two largest generated clients (SSM, Glue) pushes them past the JVM class-size limit.Testing
License