Skip to content

Conversation

@javanna
Copy link
Member

@javanna javanna commented Dec 20, 2016

If we conditionally do random things, e.g. initialize a node only after the first test, we have to make sure that we unconditionally create a new seed calling random.nextLong(), then initialize the node under a private randomness context. This makes sure that any random usage through Randomness.get() will retrieve the proper random instance through RandomizedContext.current().getRandom(). When running under private randomness, the context will return the Random instance that was created with the provided seed (forked from the main random instance) rather than the main Random that's exposed to tests as well. Otherwise tests become non repeatable because that initialization part happens only before the first executed test.

If we conditionally do random things, e.g. initialize a node only after the first test, we have to make sure that we fork the Random instance by calling random.nextLong() unconditionally, otherwise tests with same seed may not be repeatable.

There is one problem left out of this PR, in IngestActionForwarder, relates to elastic#22282 . That is the only reason left why subclasses of ESSingleNodeTestCase are not repeatable yet.
@javanna javanna added review >test Issues or PRs that are addressing/adding tests v5.2.0 v6.0.0-alpha1 labels Dec 20, 2016
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

Good catch!

@javanna
Copy link
Member Author

javanna commented Dec 20, 2016

I pushed a new commit and updated the description. I am not using runWithPrivateRandomness which fixes all the issues I've seen with repeatability.

Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

LGTM.

@javanna javanna merged commit ae01a51 into elastic:master Dec 21, 2016
@javanna javanna added the v5.1.2 label Dec 21, 2016
@javanna javanna added the v5.0.3 label Dec 21, 2016
javanna added a commit to javanna/elasticsearch that referenced this pull request Dec 21, 2016
If we conditionally do random things, e.g. initialize a node only after the first test, we have to make sure that we unconditionally create a new seed calling random.nextLong(), then initialize the node under a private randomness context. This makes sure that any random usage through Randomness.get() will retrieve the proper random instance through RandomizedContext.current().getRandom(). When running under private randomness, the context will return the Random instance that was created with the provided seed (forked from the main random instance) rather than the main Random that's exposed to tests as well. Otherwise tests become non repeatable because that initialization part happens only before the first executed test.
javanna added a commit to javanna/elasticsearch that referenced this pull request Dec 21, 2016
If we conditionally do random things, e.g. initialize a node only after the first test, we have to make sure that we unconditionally create a new seed calling random.nextLong(), then initialize the node under a private randomness context. This makes sure that any random usage through Randomness.get() will retrieve the proper random instance through RandomizedContext.current().getRandom(). When running under private randomness, the context will return the Random instance that was created with the provided seed (forked from the main random instance) rather than the main Random that's exposed to tests as well. Otherwise tests become non repeatable because that initialization part happens only before the first executed test.
@javanna javanna removed the v5.0.3 label Dec 21, 2016
javanna added a commit that referenced this pull request Dec 21, 2016
If we conditionally do random things, e.g. initialize a node only after the first test, we have to make sure that we unconditionally create a new seed calling random.nextLong(), then initialize the node under a private randomness context. This makes sure that any random usage through Randomness.get() will retrieve the proper random instance through RandomizedContext.current().getRandom(). When running under private randomness, the context will return the Random instance that was created with the provided seed (forked from the main random instance) rather than the main Random that's exposed to tests as well. Otherwise tests become non repeatable because that initialization part happens only before the first executed test.
javanna added a commit that referenced this pull request Dec 21, 2016
If we conditionally do random things, e.g. initialize a node only after the first test, we have to make sure that we unconditionally create a new seed calling random.nextLong(), then initialize the node under a private randomness context. This makes sure that any random usage through Randomness.get() will retrieve the proper random instance through RandomizedContext.current().getRandom(). When running under private randomness, the context will return the Random instance that was created with the provided seed (forked from the main random instance) rather than the main Random that's exposed to tests as well. Otherwise tests become non repeatable because that initialization part happens only before the first executed test.
@jasontedor
Copy link
Member

Nice one @javanna.

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Dec 21, 2016
* master:
  Simplify Unicast Zen Ping (elastic#22277)
  Replace IndicesQueriesRegistry (elastic#22289)
  Fixed document mistake and fit for 5.1.1 API
  [TEST] improve error message in ESTestCase#assertWarnings
  [TEST] remove deleted test classes from checkstyle suppressions
  [TEST] make ESSingleNodeTestCase tests repeatable (elastic#22283)
  Link for setting page in elasticsearch.yml is outdated
  Factor out sort values from InternalSearchHit (elastic#22080)
  Add ID for percolate query to Java API docs
  x_refresh.yaml tests should use unique index names and doc ids to ease debugging
  IndicesStoreIntegrationIT should not use start recovery sending as an indication that the recovery started
  Added base class for testing aggregators and some initial tests for `terms`, `top_hits` and `min` aggregations.
  Add link to foreach processor to ingest-attachment.asciidoc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>test Issues or PRs that are addressing/adding tests v5.1.2 v5.2.0 v6.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants