Skip to content
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

Ensure global test seed is used for all random testing tasks #38991

Merged
merged 2 commits into from
Feb 16, 2019

Conversation

mark-vieira
Copy link
Contributor

Recently it was noticed that the global generated test seed reported at the beginning of every Gradle build was not always used for subsequent test tasks. For example:

=======================================
Elasticsearch Build Hamster says Hello!
  Gradle Version        : 5.2.1
  OS Info               : Mac OS X 10.14.3 (x86_64)
  JDK Version           : 11 (Oracle Corporation 11.0.2 [OpenJDK 64-Bit Server VM 11.0.2+9])
  JAVA_HOME             : /Users/mark/.sdkman/candidates/java/11.0.2-open
  Random Testing Seed   : DA01B1B09A1CDA37
=======================================

> Task :server:integTest
==> Test Info: seed=4804DC280933D0C4; jvms=6; suites=319

As you can see, the global seed DA01B1B09A1CDA37 is not the same as the seed 4804DC280933D0C4 used by the :server:integTest Task.

It turns out the reason for this is an implementation detail in how the Ant junit4 task works.

// Initialize random if not already provided.
    if (random == null) {
      this.random = MoreObjects.firstNonNull( 
          Strings.emptyToNull(getProject().getProperty(SYSPROP_RANDOM_SEED())),
          SeedUtils.formatSeed(new Random().nextLong()));
    }

In this case it looks for an Ant project property, and if not defined, falls back to generating a new seed.

We currently rely on explicitly setting the test.seed system property on the test task here but this actually gets ignored when the forked test VMs are created and the randomly generated seed (created above) is used instead.

// Pass the random seed property.
    createJvmarg().setValue("-D" + SYSPROP_PREFIX() + "=" + CURRENT_PREFIX());
    createJvmarg().setValue("-D" + SYSPROP_RANDOM_SEED() + "=" + random);

While investigating it was noticed that invoking the build with an explicit seed (ex: ./gradlew -Dtest.seed=foo) does result in the behavior we want, which is that all random test tasks in the build use the global seed. The reason for this is that all build system properties are inherited by Ant as project properties.

This PR simply unifies these behaviors such that when the global seed is generated as part of the build (not specified on the commandline) we explicitly set the appropriate Ant project property to ensure this propagates to any and all Ant tasks.

This commit fixes a bug which resulted in every RandomizedTestingTask
generating its own unique test seed rather than using the global test
seed generated by the the RandomizedTestingPlugin.

The issue didn't affect build invocations which explicitly ran with
-Dtest.seed. In those cases, the Ant junit task correctly uses the
global seed.

Since the Ant random testing target looks for an Ant project property
for determining the existing seed we now explicitly set this inside
RandomizedTestingPlugin. It just so happens that, incidentally, Ant will
inherit system properties, thus wy the -Dtest.seed mechanism continued
to work.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@mark-vieira
Copy link
Contributor Author

@elasticmachine retest this please

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one nit

import org.gradle.api.tasks.TaskContainer

class RandomizedTestingPlugin implements Plugin<Project> {

void apply(Project project) {
setupSeed(project)
def seed = setupSeed(project)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we try to use concrete types, as it makes it easier to port these classes to java (which we have slowly been doing).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then when we port from Java -> Kotlin we'll have to change back to val 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️ joking about porting to Kotlin FYI 😆

@mark-vieira mark-vieira merged commit 05a3108 into elastic:master Feb 16, 2019
@mark-vieira mark-vieira deleted the consistent-test-seed branch February 16, 2019 03:36
jasontedor pushed a commit to jasontedor/elasticsearch that referenced this pull request Feb 16, 2019
…#38991)

This commit fixes a bug which resulted in every RandomizedTestingTask
generating its own unique test seed rather than using the global test
seed generated by the the RandomizedTestingPlugin.

The issue didn't affect build invocations which explicitly ran with
-Dtest.seed. In those cases, the Ant junit task correctly uses the
global seed.

Since the Ant random testing target looks for an Ant project property
for determining the existing seed we now explicitly set this inside
RandomizedTestingPlugin. It just so happens that, incidentally, Ant will
inherit system properties, thus wy the -Dtest.seed mechanism continued
to work.

* Use explicit type
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 16, 2019
* elastic/master:
  Ensure global test seed is used for all random testing tasks (elastic#38991)
  re-mutes SmokeTestWatcherWithSecurityIT (elastic#38995)
  Rollup jobs should be cleaned up before indices are deleted (elastic#38930)
  relax ML Info Docs expected response (elastic#38993)
  Re-enable single node tests (elastic#38852)
  ClusterClientIT refactor (elastic#38872)
  Fix typo in Index API doc
  Edits to text & formatting in Term Suggester doc (elastic#38963) (elastic#38989)
  Migrate Streamable to Writeable for WatchStatus (elastic#37390)
mark-vieira added a commit to mark-vieira/elasticsearch that referenced this pull request Feb 20, 2019
…#38991)

This commit fixes a bug which resulted in every RandomizedTestingTask
generating its own unique test seed rather than using the global test
seed generated by the the RandomizedTestingPlugin.

The issue didn't affect build invocations which explicitly ran with
-Dtest.seed. In those cases, the Ant junit task correctly uses the
global seed.

Since the Ant random testing target looks for an Ant project property
for determining the existing seed we now explicitly set this inside
RandomizedTestingPlugin. It just so happens that, incidentally, Ant will
inherit system properties, thus wy the -Dtest.seed mechanism continued
to work.

* Use explicit type
mark-vieira added a commit to mark-vieira/elasticsearch that referenced this pull request Feb 20, 2019
…#38991)

This commit fixes a bug which resulted in every RandomizedTestingTask
generating its own unique test seed rather than using the global test
seed generated by the the RandomizedTestingPlugin.

The issue didn't affect build invocations which explicitly ran with
-Dtest.seed. In those cases, the Ant junit task correctly uses the
global seed.

Since the Ant random testing target looks for an Ant project property
for determining the existing seed we now explicitly set this inside
RandomizedTestingPlugin. It just so happens that, incidentally, Ant will
inherit system properties, thus wy the -Dtest.seed mechanism continued
to work.

* Use explicit type
mark-vieira added a commit to mark-vieira/elasticsearch that referenced this pull request Feb 20, 2019
…#38991)

This commit fixes a bug which resulted in every RandomizedTestingTask
generating its own unique test seed rather than using the global test
seed generated by the the RandomizedTestingPlugin.

The issue didn't affect build invocations which explicitly ran with
-Dtest.seed. In those cases, the Ant junit task correctly uses the
global seed.

Since the Ant random testing target looks for an Ant project property
for determining the existing seed we now explicitly set this inside
RandomizedTestingPlugin. It just so happens that, incidentally, Ant will
inherit system properties, thus wy the -Dtest.seed mechanism continued
to work.

* Use explicit type
mark-vieira added a commit to mark-vieira/elasticsearch that referenced this pull request Feb 21, 2019
…#38991)

This commit fixes a bug which resulted in every RandomizedTestingTask
generating its own unique test seed rather than using the global test
seed generated by the the RandomizedTestingPlugin.

The issue didn't affect build invocations which explicitly ran with
-Dtest.seed. In those cases, the Ant junit task correctly uses the
global seed.

Since the Ant random testing target looks for an Ant project property
for determining the existing seed we now explicitly set this inside
RandomizedTestingPlugin. It just so happens that, incidentally, Ant will
inherit system properties, thus wy the -Dtest.seed mechanism continued
to work.

* Use explicit type
mark-vieira added a commit that referenced this pull request Feb 21, 2019
…#39196)

This commit fixes a bug which resulted in every RandomizedTestingTask
generating its own unique test seed rather than using the global test
seed generated by the the RandomizedTestingPlugin.

The issue didn't affect build invocations which explicitly ran with
-Dtest.seed. In those cases, the Ant junit task correctly uses the
global seed.

Since the Ant random testing target looks for an Ant project property
for determining the existing seed we now explicitly set this inside
RandomizedTestingPlugin. It just so happens that, incidentally, Ant will
inherit system properties, thus wy the -Dtest.seed mechanism continued
to work.

* Use explicit type
mark-vieira added a commit that referenced this pull request Feb 21, 2019
…#39197)

This commit fixes a bug which resulted in every RandomizedTestingTask
generating its own unique test seed rather than using the global test
seed generated by the the RandomizedTestingPlugin.

The issue didn't affect build invocations which explicitly ran with
-Dtest.seed. In those cases, the Ant junit task correctly uses the
global seed.

Since the Ant random testing target looks for an Ant project property
for determining the existing seed we now explicitly set this inside
RandomizedTestingPlugin. It just so happens that, incidentally, Ant will
inherit system properties, thus wy the -Dtest.seed mechanism continued
to work.

* Use explicit type
mark-vieira added a commit that referenced this pull request Feb 21, 2019
…#39195)

This commit fixes a bug which resulted in every RandomizedTestingTask
generating its own unique test seed rather than using the global test
seed generated by the the RandomizedTestingPlugin.

The issue didn't affect build invocations which explicitly ran with
-Dtest.seed. In those cases, the Ant junit task correctly uses the
global seed.

Since the Ant random testing target looks for an Ant project property
for determining the existing seed we now explicitly set this inside
RandomizedTestingPlugin. It just so happens that, incidentally, Ant will
inherit system properties, thus wy the -Dtest.seed mechanism continued
to work.
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team v6.7.0 v7.0.0-rc2 v7.2.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants