Skip to content

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

Merged
mark-vieira merged 2 commits intoelastic:masterfrom
mark-vieira:consistent-test-seed
Feb 16, 2019
Merged

Ensure global test seed is used for all random testing tasks#38991
mark-vieira merged 2 commits intoelastic:masterfrom
mark-vieira:consistent-test-seed

Conversation

@mark-vieira
Copy link
Copy Markdown
Contributor

@mark-vieira mark-vieira commented Feb 15, 2019

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.

@mark-vieira
Fix inconsistent test seed generation
544936c 
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 self-assigned this Feb 15, 2019
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Feb 15, 2019

Pinging @elastic/es-core-infra

@mark-vieira
Copy link
Copy Markdown
Contributor Author

mark-vieira commented Feb 15, 2019

@elasticmachine retest this please

rjernst
rjernst approved these changes Feb 15, 2019
View reviewed changes
Copy link
Copy Markdown
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

buildSrc/src/main/groovy/com/carrotsearch/gradle/junit4/RandomizedTestingPlugin.groovy Outdated

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

@rjernst rjernst Feb 15, 2019

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
Copy Markdown
Contributor Author

@mark-vieira mark-vieira Feb 15, 2019

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
Copy Markdown
Contributor Author

@mark-vieira mark-vieira Feb 15, 2019

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 added a commit to jasontedor/elasticsearch that referenced this pull request Feb 16, 2019
@jasontedor
Merge remote-tracking branch 'elastic/master' into retention-lease-ccr
2c77ad4 
* 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
@mark-vieira
Ensure global test seed is used for all random testing tasks (elastic…
fa53f66 
…#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
@mark-vieira
Ensure global test seed is used for all random testing tasks (elastic…
d3167ca 
…#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
@mark-vieira
Ensure global test seed is used for all random testing tasks (elastic…
2c18310 
…#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
@mark-vieira
Ensure global test seed is used for all random testing tasks (#38991) (…
8e90d8d 
…#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
@mark-vieira
Ensure global test seed is used for all random testing tasks (#38991) (…
ecf7bb0 
…#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
@mark-vieira
Ensure global test seed is used for all random testing tasks (#38991) (…
0f68814 
…#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

Reviewers

@rjernst rjernst rjernst approved these changes

@alpar-t alpar-t Awaiting requested review from alpar-t

Assignees

@mark-vieira mark-vieira

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mark-vieira @elasticmachine @rjernst @jakelandis