Use of Random in dunit tests

[Kirk Lund <kl...@pivotal.io>]

GemfireDataCommandsDUnitTest is one of the dunit tests I have migrated to
junit4 as part of GEODE-1162.

These two import jumped out at me immediately:

import hydra.GsRandom;
import java.util.Random;

If you really want to see how they're used in the class, go read the src
code, but I'll summarize here.

The test is randomly test one way out of two different modes. This is bad
because this means that out of any given test run, one mode/path/whatever
has no test coverage. I changed that to test BOTH ways every time.

The test is also using Random to generate random integers. I think this is
worse than using magic numbers. None of our unit/integration/distributed
tests should have any randomization of any sort.

If you run into stuff like this please: 1) remove hydra.GsRandom at a
minimum, 2) replace any randomization to make the test(s) deterministic and
with non-random test coverage.

-Kirk

Re: Use of Random in dunit tests

[Kirk Lund <kl...@pivotal.io>]

To help give you ideas in case you hit something similar... Here's the
original code:

public void testSelectCommand() {
  ...
  doTestSelectWithGfshEnvVariables();

public void doTestSelectWithGfshEnvVariables() {
  ...
  executeCommand("set variable --name=STATUS --value=" + *(new
GsRandom().nextBoolean() ? "active" : "inactive")*);


Easiest change was to do this:

public void testSelectCommand() {
  ...
  doTestSelectWithGfshEnvVariables(*true*);
  doTestSelectWithGfshEnvVariables(*false*);

public void doTestSelectWithGfshEnvVariables(*boolean statusActive*) {
  executeCommand("set variable --name=STATUS --value=" +* (statusActive ?
"active" : "inactive")*);


I didn't spend time trying to analyze whether testing this with both values
is truly valuable or not. My goal was simply to make it to be 100%
repeatable.

-Kirk


On Tue, Apr 5, 2016 at 11:53 AM, Kirk Lund <kl...@pivotal.io> wrote:

> GemfireDataCommandsDUnitTest is one of the dunit tests I have migrated to
> junit4 as part of GEODE-1162.
>
> These two import jumped out at me immediately:
>
> import hydra.GsRandom;
> import java.util.Random;
>
> If you really want to see how they're used in the class, go read the src
> code, but I'll summarize here.
>
> The test is randomly test one way out of two different modes. This is bad
> because this means that out of any given test run, one mode/path/whatever
> has no test coverage. I changed that to test BOTH ways every time.
>
> The test is also using Random to generate random integers. I think this is
> worse than using magic numbers. None of our unit/integration/distributed
> tests should have any randomization of any sort.
>
> If you run into stuff like this please: 1) remove hydra.GsRandom at a
> minimum, 2) replace any randomization to make the test(s) deterministic and
> with non-random test coverage.
>
> -Kirk
>
>