-
Notifications
You must be signed in to change notification settings - Fork 25.5k
[Gradle] Fix test seed handling when running with cc #133811
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.
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
Conversation
Pinging @elastic/es-delivery (Team:Delivery) |
} | ||
|
||
// randomise between sniff and proxy modes | ||
boolean proxyMode = buildParams.random.nextBoolean() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this resolves test seed early basically breaking configuration cache reusability every time
testSeed = testSeedProperty; | ||
} | ||
return testSeed; | ||
private Provider<String> getTestSeed() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to be able to pass a changing value to Test tasks without breaking configuration cache reuse, we have to rely on a ValueSource here that we pass down all the way to "noninputproperties" CommandLineProvider we configure for tests tasks. ValueSource allows being opaque to the configuration cache. Other providers or plain string would always be interpreted as inputs to the configuration cache, resulting in the test seed not changing between multiple build invocation without configuration cache change
} | ||
|
||
// randomise between sniff and proxy modes | ||
boolean proxyMode = buildParams.random.nextBoolean() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we only use this in deprecated TestCluster settings I did not invest to much time to sugar code that api here for now. Ultimately this moves into tests at one point 🤞
775cc35
to
6a9f399
Compare
This ensures we provide a new test seed per build invocation when running with configuration cache enabled while still ensuring the configuration cache can be reused. basically test seed beeing ignored as cc input.
6a9f399
to
769fbb3
Compare
javaRestTestImplementation project(':x-pack:plugin:security') | ||
} | ||
|
||
boolean literalUsername = buildParams.random.nextBoolean() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buildParams.random resolves the TestSeed and breaks configuration-cache. by moving this into the task configuration block we defer that. for this particular test cc will always be out of date. Longterm we might wand a more deterministic way of testing this behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The altenative would be to split the random from the testseed,, but this would break reproducability.
this.javaInstallationRegistry = javaInstallationRegistry; | ||
this.metadataDetector = new ErrorTraceMetadataDetector(metadataDetector); | ||
this.providers = providers; | ||
this.branchesFileParser = new BranchesFileParser(new ObjectMapper()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems only used at one point. therefore I removed the field property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I got the impression that it belongs in constructor, to avoid creating it on every method call, but the apply method will be called only once anyway.
this.project = project; | ||
project.getPlugins().apply(JvmToolchainsPlugin.class); | ||
project.getPlugins().apply(JdkDownloadPlugin.class); | ||
project.getPlugins().apply(VersionPropertiesPlugin.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to follow the pattern of applying nested plugins as early as possible (there are edge cases where you might only do it on certain conditions) in the apply method. that makes reading way simpler and I think its common practice
|
||
if (buildFeatures.getConfigurationCache().getActive().get() == false) { | ||
// if configuration cache is enabled, resolving the test seed early breaks configuration cache reuse | ||
LOGGER.quiet(" Random Testing Seed : " + buildParams.getTestSeed()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test seed printing resolves the ValueSource early and breaks configuration cache. therefore only print if not running in cc
import java.util.Locale; | ||
import java.util.Random; | ||
|
||
public abstract class TestSeedValueSource implements ValueSource<String, ValueSourceParameters.None> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to model the TestSeed resolution as ValueSource as this allows passing the parameter transparently for the configuration cache down to the consumers (test cases, testcluster).
Taking this from the according javadoc:
Represents an external source of information used by a Gradle build. Examples of external sources include client environment variables, system properties, configuration files, shell commands, network services, among others.
Representing external sources as [ValueSource](https://docs.gradle.org/current/javadoc/org/gradle/api/provider/ValueSource.html)s allows Gradle to transparently manage [the configuration cache](https://docs.gradle.org/current/userguide/configuration_cache.html) as values obtained from those sources change. For example, a build might run a different set of tasks depending on whether the CI environment variable is set or not.
This ensures we provide a new test seed per build invocation when running with configuration cache enabled while still ensuring the configuration cache can be reused. basically test seed beeing ignored as cc input.
💔 Backport failed
You can use sqren/backport to manually backport by running |
This ensures we provide a new test seed per build invocation when running with configuration cache enabled while still ensuring the configuration cache can be reused. basically test seed beeing ignored as cc input. (cherry picked from commit 8097c1a) # Conflicts: # x-pack/plugin/security/qa/profile/build.gradle
This ensures we provide a new test seed per build invocation when running with configuration cache enabled while still ensuring the configuration cache can be reused. basically test seed beeing ignored as cc input. (cherry picked from commit 8097c1a) # Conflicts: # x-pack/plugin/security/qa/profile/build.gradle
This ensures we provide a new test seed per build invocation when running with configuration cache enabled while still ensuring the configuration cache can be reused. basically test seed beeing ignored as cc input. (cherry picked from commit 8097c1a) # Conflicts: # x-pack/plugin/security/qa/profile/build.gradle
💔 Some backports could not be created
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
This ensures we provide a new test seed per build invocation when running with configuration cache enabled while still ensuring the configuration cache can be reused. basically test seed beeing ignored as cc input. (cherry picked from commit 8097c1a) # Conflicts: # x-pack/plugin/security/qa/profile/build.gradle
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
This ensures we provide a new test seed per build invocation when running with configuration cache enabled while still ensuring the configuration cache can be reused. basically test seed beeing ignored as cc input. (cherry picked from commit 8097c1a) # Conflicts: # x-pack/plugin/security/qa/profile/build.gradle
This ensures we provide a new test seed per build invocation when running with configuration cache enabled while still ensuring the configuration cache can be reused. basically test seed beeing ignored as cc input. (cherry picked from commit 8097c1a) # Conflicts: # x-pack/plugin/security/qa/profile/build.gradle
This ensures we provide a new test seed per build invocation when running with configuration cache enabled while still ensuring the configuration cache can be reused. basically test seed beeing ignored as cc input. (cherry picked from commit 8097c1a) # Conflicts: # x-pack/plugin/security/qa/profile/build.gradle
…#134149) * [Gradle] Fix test seed handling when running with cc (#133811) This ensures we provide a new test seed per build invocation when running with configuration cache enabled while still ensuring the configuration cache can be reused. basically test seed beeing ignored as cc input. (cherry picked from commit 8097c1a) # Conflicts: # x-pack/plugin/security/qa/profile/build.gradle * Fix merge issue
BASE=24bd70b9d1eb60747a7ba3b7c971e2096da843fd HEAD=a25ede51f680602cdeeae9b45d1540ca9fbcc04f Branch=main
BASE=24bd70b9d1eb60747a7ba3b7c971e2096da843fd HEAD=a25ede51f680602cdeeae9b45d1540ca9fbcc04f Branch=main
…elastic#134147) This ensures we provide a new test seed per build invocation when running with configuration cache enabled while still ensuring the configuration cache can be reused. basically test seed beeing ignored as cc input. (cherry picked from commit 8097c1a) # Conflicts: # x-pack/plugin/security/qa/profile/build.gradle
BASE=24bd70b9d1eb60747a7ba3b7c971e2096da843fd HEAD=a25ede51f680602cdeeae9b45d1540ca9fbcc04f Branch=main
BASE=24bd70b9d1eb60747a7ba3b7c971e2096da843fd HEAD=a25ede51f680602cdeeae9b45d1540ca9fbcc04f Branch=main
BASE=24bd70b9d1eb60747a7ba3b7c971e2096da843fd HEAD=a25ede51f680602cdeeae9b45d1540ca9fbcc04f Branch=main
…elastic#134147) This ensures we provide a new test seed per build invocation when running with configuration cache enabled while still ensuring the configuration cache can be reused. basically test seed beeing ignored as cc input. (cherry picked from commit 8097c1a) # Conflicts: # x-pack/plugin/security/qa/profile/build.gradle
BASE=24bd70b9d1eb60747a7ba3b7c971e2096da843fd HEAD=a25ede51f680602cdeeae9b45d1540ca9fbcc04f Branch=main
This ensures we provide a new test seed per build invocation when running
with configuration cache enabled while still ensuring the configuration cache
can be reused.
basically test seed beeing ignored as cc input.