Mirror upstream elastic/elasticsearch#133811 for AI review (snapshot of HEAD tree) #143
Conversation
BASE=24bd70b9d1eb60747a7ba3b7c971e2096da843fd HEAD=a25ede51f680602cdeeae9b45d1540ca9fbcc04f Branch=main
WalkthroughIntroduces provider-based test seed and randomness handling across plugins and build scripts, integrates BuildFeatures checks, adds a ValueSource to supply tests.seed, updates GlobalBuildInfoPlugin and ElasticsearchTestBasePlugin to use providers, adjusts x-pack build scripts to map randomness via providers, and adds a functional test verifying per-invocation test seeds with configuration cache reuse. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Gradle as Gradle Build
participant GBIP as GlobalBuildInfoPlugin
participant ETP as ElasticsearchTestBasePlugin
participant BF as BuildFeatures
participant VS as TestSeedValueSource
participant Task as Test Task(s)
Dev->>Gradle: Run build (with/without config cache)
Gradle->>GBIP: apply()
GBIP->>BF: isConfigurationCacheActive()
alt CC active
note over GBIP,Task: Defer seed resolution via Provider
else CC inactive
GBIP->>VS: obtain tests.seed (provider resolve)
VS-->>GBIP: seed (from sysprop or generated)
GBIP-->>Gradle: Log seed (non-CC path)
end
Gradle->>ETP: configure test tasks
ETP->>BF: isConfigurationCacheActive() / ignore.tests.seed?
alt Use Provider path
ETP->>Task: systemProperty "tests.seed" <- buildParams.getTestSeedProvider()
else Direct path
ETP->>Task: systemProperty "tests.seed" <- buildParams.getTestSeed()
end
loop Within a single invocation
Task->>Task: All tests use same provided seed
end
note over Gradle: Next invocation gets a new Provider value (different seed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
x-pack/qa/multi-cluster-search-security/legacy-with-basic-license/build.gradle (1)
60-69: Avoid multiple.get()calls on the random providerEach call to
proxyModeProvider.get()re-runs themapclosure, advancing the sharedRandomand yielding different booleans. The cluster configuration and blacklist can therefore diverge (e.g. cluster configured for proxy mode while the task assumes sniff), breaking test setup nondeterministically. Resolve the provider once and reuse the same value everywhere.-def proxyModeProvider = buildParams.random.map{r -> r.nextBoolean()} +def proxyModeProvider = buildParams.random.map { r -> r.nextBoolean() } +def proxyMode = proxyModeProvider.get() ... - if (proxyModeProvider.get()) { + if (proxyMode) { ... - if (proxyModeProvider.get()) { + if (proxyMode) {Also applies to: 82-89
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/ElasticsearchTestBasePluginFuncTest.groovy(1 hunks)build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/ElasticsearchTestBasePlugin.java(3 hunks)build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/info/BuildParameterExtension.java(2 hunks)build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/info/DefaultBuildParameterExtension.java(4 hunks)build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/info/GlobalBuildInfoPlugin.java(7 hunks)build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/info/TestSeedValueSource.java(1 hunks)x-pack/plugin/security/qa/profile/build.gradle(1 hunks)x-pack/plugin/sql/qa/jdbc/security/build.gradle(0 hunks)x-pack/qa/multi-cluster-search-security/legacy-with-basic-license/build.gradle(3 hunks)x-pack/qa/multi-cluster-search-security/legacy-with-full-license/build.gradle(2 hunks)x-pack/qa/multi-cluster-search-security/legacy-with-restricted-trust/build.gradle(2 hunks)
💤 Files with no reviewable changes (1)
- x-pack/plugin/sql/qa/jdbc/security/build.gradle
| verifyAll { | ||
| seeds1[0] != null | ||
| seeds2[0] != null | ||
| seeds1[0] != seeds2[0] | ||
| } |
There was a problem hiding this comment.
Guard against empty seed lists before indexing
If neither test nor test2 emits the TESTSEED=[…] line, seeds1[0]/seeds2[0] will throw IndexOutOfBoundsException. Please assert the lists are non-empty before indexing, e.g. !seeds1.isEmpty() / !seeds2.isEmpty().
🤖 Prompt for AI Agents
In
build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/ElasticsearchTestBasePluginFuncTest.groovy
around lines 165 to 169, guard against empty seed lists before indexing by first
asserting both lists are non-empty (e.g. !seeds1.isEmpty() and
!seeds2.isEmpty()) and only then index into [0] for null checks and comparison;
update the verifyAll block to check !seeds1.isEmpty() && !seeds2.isEmpty()
before evaluating seeds1[0] != null, seeds2[0] != null and seeds1[0] !=
seeds2[0].
| @Override | ||
| public ValueSourceParameters.None getParameters() { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Return the ValueSourceParameters.None singleton instead of null.
Using the provided singleton clarifies intent and avoids any accidental null-handling edge cases inside Gradle’s value source plumbing.
@Override
public ValueSourceParameters.None getParameters() {
- return null;
+ return ValueSourceParameters.None.INSTANCE;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Override | |
| public ValueSourceParameters.None getParameters() { | |
| return null; | |
| } | |
| @Override | |
| public ValueSourceParameters.None getParameters() { | |
| return ValueSourceParameters.None.INSTANCE; | |
| } |
🤖 Prompt for AI Agents
In
build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/info/TestSeedValueSource.java
around lines 20 to 23, the getParameters() method currently returns null;
replace that null with the provided singleton by returning
ValueSourceParameters.None.INSTANCE instead so the method returns the
appropriate non-null singleton value and avoids null-handling issues in Gradle's
value source plumbing.
| tasks.named("javaRestTest").configure { | ||
| usesDefaultDistribution("to be triaged") | ||
| systemProperty 'test.literalUsername', literalUsername | ||
| systemProperty 'test.literalUsername', buildParams.random.map{r -> r.nextBoolean()}.get() |
There was a problem hiding this comment.
Don't eagerly resolve the random provider.
Calling .get() here forces the provider to resolve during configuration, which locks the boolean into the configuration cache and still reuses the same value on subsequent invocations. Use the provider directly (via systemProperty’s provider overload or systemPropertyProviders) so the boolean is generated when the task runs.
- systemProperty 'test.literalUsername', buildParams.random.map{ r -> r.nextBoolean() }.get()
+ systemPropertyProviders.put('test.literalUsername', buildParams.random.map { r -> Boolean.toString(r.nextBoolean()) })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| systemProperty 'test.literalUsername', buildParams.random.map{r -> r.nextBoolean()}.get() | |
| systemPropertyProviders.put('test.literalUsername', buildParams.random.map { r -> Boolean.toString(r.nextBoolean()) }) |
🤖 Prompt for AI Agents
In x-pack/plugin/security/qa/profile/build.gradle around line 17, the call to
buildParams.random.map{r -> r.nextBoolean()}.get() eagerly resolves the provider
at configuration time; change it to pass the Provider itself to the system
property (use the systemProperty overload that accepts a Provider or use
systemPropertyProviders) so the boolean is produced at execution time rather
than calling .get() during configuration.
|
|
||
| // randomise between sniff and proxy modes | ||
| boolean proxyMode = buildParams.random.nextBoolean() | ||
| var proxyModeProvider = buildParams.random.map{r -> r.nextBoolean()} |
There was a problem hiding this comment.
Fix Groovy variable declaration
var is not valid in Groovy build scripts and causes unable to resolve class var. Declare the provider with def (or an explicit Provider<Boolean>).
-var proxyModeProvider = buildParams.random.map{r -> r.nextBoolean()}
+def proxyModeProvider = buildParams.random.map { r -> r.nextBoolean() }🤖 Prompt for AI Agents
In
x-pack/qa/multi-cluster-search-security/legacy-with-basic-license/build.gradle
around line 22, the script uses the invalid Groovy keyword "var" causing "unable
to resolve class var"; replace the declaration with a Groovy-compatible one such
as "def proxyModeProvider = buildParams.random.map { r -> r.nextBoolean() }" or
an explicit typed provider like "Provider<Boolean> proxyModeProvider =
buildParams.random.map { r -> r.nextBoolean() }" so the variable is declared
correctly and the build script resolves.
|
|
||
| // randomise between sniff and proxy modes | ||
| boolean proxyMode = buildParams.random.nextBoolean() | ||
| var proxyModeProvider = buildParams.random.map{r -> r.nextBoolean()} |
There was a problem hiding this comment.
Replace var with a Groovy-compatible declaration
build.gradle uses the Groovy DSL, which does not recognize var as a type. This line will fail with unable to resolve class var. Declare the provider with def (or an explicit Provider<Boolean> type) instead.
-var proxyModeProvider = buildParams.random.map{r -> r.nextBoolean()}
+def proxyModeProvider = buildParams.random.map { r -> r.nextBoolean() }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var proxyModeProvider = buildParams.random.map{r -> r.nextBoolean()} | |
| def proxyModeProvider = buildParams.random.map { r -> r.nextBoolean() } |
🤖 Prompt for AI Agents
In x-pack/qa/multi-cluster-search-security/legacy-with-full-license/build.gradle
around line 22, the declaration uses the Java/Kotlin-style keyword "var" which
Groovy DSL doesn't recognize; replace "var" with a Groovy-compatible declaration
such as "def proxyModeProvider = buildParams.random.map { r -> r.nextBoolean()
}" or declare an explicit Provider<Boolean> type so the build resolves
correctly.
|
|
||
| // randomise between sniff and proxy modes | ||
| boolean proxyMode = buildParams.random.nextBoolean() | ||
| var proxyModeProvider = buildParams.random.map{r -> r.nextBoolean()} |
There was a problem hiding this comment.
Use def instead of var in Groovy build scripts
Groovy DSL scripts cannot declare variables with var; this triggers unable to resolve class var at configuration time. Switch to def (or an explicit Provider<Boolean> type).
-var proxyModeProvider = buildParams.random.map{r -> r.nextBoolean()}
+def proxyModeProvider = buildParams.random.map { r -> r.nextBoolean() }🤖 Prompt for AI Agents
In
x-pack/qa/multi-cluster-search-security/legacy-with-restricted-trust/build.gradle
around line 32, the Groovy build script incorrectly uses the JavaScript/Java
'var' keyword which causes "unable to resolve class var" at configuration time;
replace 'var' with 'def' (or an explicit Provider<Boolean> type) so the
declaration becomes valid Groovy and the expression buildParams.random.map{ r ->
r.nextBoolean() } is assigned correctly.
1 participant
Single commit with tree=a25ede51f680602cdeeae9b45d1540ca9fbcc04f^{tree}, parent=24bd70b9d1eb60747a7ba3b7c971e2096da843fd. Exact snapshot of upstream PR head. No conflict resolution attempted.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores