Skip to content

High level REST client test framework encourages using randomness for test coverage #39667

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

Closed
jasontedor opened this issue Mar 4, 2019 · 3 comments · Fixed by #48211
Closed
Assignees

Comments

@jasontedor
Copy link
Member

We want to avoid using randomness for test coverage. Randomness is good in cases when the values are not expected to impact the behavior of the functionality under test (e.g., randomizing the number of shards, whether or not replicas are present, or the store type in most cases is not expected to have any impact on functionality). In these cases, using randomness ensures that we are in fact not relying on implementation details of components unrelated to the functionality under test. However we want to avoid using randomness to ensure branch coverage, as it leads to tests that only run a fraction of the time, and so we can end up with a test passing (e.g., on a PR), only to fail later in CI. Instead, we want distinct test cases.

The high level REST client test framework encourages using randomness to ensure test coverage of sync versus async method execution. That is, the test framework randomly tests sync as opposed to async methods, only half the time covering one and half the time the other. Some tests that are invoking multiple high level REST clients in a single test end up with a confusing mix of sync and async requests.

Instead, we would want to refactor these tests so that on every test run we are covering both sync and async methods.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@hub-cap
Copy link
Contributor

hub-cap commented Mar 8, 2019

++. Ive commented out this bit of code more than once when i was testing something I was modifying in the framework. Ill turn this issue into a meta that has more than just the sync/async randomness portion.

@hub-cap hub-cap self-assigned this Mar 8, 2019
@hub-cap
Copy link
Contributor

hub-cap commented Oct 17, 2019

I looked into the framework for the rest tests, and this sync vs async is the only randomized offender for branch coverage. Ill be putting up a commit shortly that uses gradle to run the tests twice, once with all sync and once with all async.

hub-cap added a commit to hub-cap/elasticsearch that referenced this issue Oct 17, 2019
This commit removes the randomization used by every execute call in the
high level rest tests. Previously every execute call, which can be many
calls per single test, would rely on a random boolean to determine if
they should use the sync or async methods provided to the execute
method. This commit runs the tests twice, using two different clusters,
both of them providing the value one time via a sysprop. This ensures
that the whole suite of tests is run using the sync and async code
paths.

Closes elastic#39667
hub-cap added a commit that referenced this issue Oct 24, 2019
This commit removes the randomization used by every execute call in the
high level rest tests. Previously every execute call, which can be many
calls per single test, would rely on a random boolean to determine if
they should use the sync or async methods provided to the execute
method. This commit runs the tests twice, using two different clusters,
both of them providing the value one time via a sysprop. This ensures
that the whole suite of tests is run using the sync and async code
paths.

Closes #39667
hub-cap added a commit that referenced this issue Oct 24, 2019
This commit removes the randomization used by every execute call in the
high level rest tests. Previously every execute call, which can be many
calls per single test, would rely on a random boolean to determine if
they should use the sync or async methods provided to the execute
method. This commit runs the tests twice, using two different clusters,
both of them providing the value one time via a sysprop. This ensures
that the whole suite of tests is run using the sync and async code
paths.

Closes #39667
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants