Fix test reproducability in AbstractBuilderTestCase setup

[cbuescher]

Currently AbstractBuilderTestCase generates certain random values in its
beforeTest() method annotated with @before only the first time that a test
method in the suite is run. This changes the values of subsequent random values
and has the effect that when running single methods from a test suite with
"-Dtests.method=*", the random values it sees are different from when the same
test method is run as part of the whole test suite. This makes it harder to use
the reproduction lines logged on failure.

This changes the above method so that beforeTest will do the same call to the
random value source each time a test method is run, so reproduction by running
just one method is possible again.

Closes #32400

[elasticmachine]

Pinging @elastic/es-search-aggs

[jimczi]

Good catch @cbuescher , I left a comment

[jimczi]

can we initialize indexVersionCreated and now in the BeforeClass as static members like we do for nodeSetttings and index ?

[cbuescher]

Good point, I will change this for now, I don't think we need different values for now for different test cases. For indexVersionCreated I find this to be more difficult. We have at least three tests (ParentIdQueryBuilderTests, HasChildQueryBuilderTests, HasParentQueryBuilderTests) overwriting createTestIndexSettings to create a specialized version setting (it seems to be restricted to CURRENT). I'm not sure we can use this mechanism of overwriting default versions in specific tests if the call this in a static context in BeforeClass. I might have missed something though, let me know what you think.

[cbuescher]

@jimczi thanks for the review, I updated the PR by adressing one of your suggestions, left a question for the other. Let me know what you think about it.

[javanna]

The proposed fix is ok, but I wonder if the following wouldn't be better:

    @Before
    public void beforeTest() throws Exception {
        if (serviceHolder == null) {
            assert serviceHolderWithNoType == null;
            long masterSeed = SeedUtils.parseSeed(RandomizedTest.getContext().getRunnerSeedAsString());
            RandomizedTest.getContext().runWithPrivateRandomness(masterSeed, (Callable<Void>) () -> {
                serviceHolder = new ServiceHolder(nodeSettings, indexSettings(), getPlugins(), AbstractBuilderTestCase.this, true);
                serviceHolderWithNoType = new ServiceHolder(nodeSettings, indexSettings(), getPlugins(), AbstractBuilderTestCase.this, false);
                return null;
            });
        }
        serviceHolder.clientInvocationHandler.delegate = this;
        serviceHolderWithNoType.clientInvocationHandler.delegate = this;
    }

The idea is to execute this conditional initialization step under the suite randomness rather than the method randomness. If we don't want to parse the suite seed back from a string we could also generate our own suite seed, the important bit is that we generate it in the beforeClass which is executed under the suite randomness (master seed). I find that this way what we are doing is clearer. The proposed fix is good but can easily be reverted by mistake by just moving lines around.

[cbuescher]

Great suggestion, I added this.

[javanna]

I left one comment, I think it would also be good to add a test for this but it's quite tricky to do and I don't have a good idea around that. I had a quick look at some of our base test classes to see where we do similar things (conditionally initialize static variables in before methods, and generate random values as part of such init methods) and I think that ESClientYamlSuiteTestCase and ESRestTestCase have the same issue.

[cbuescher]

@javanna thanks for the suggestion, I added it in another commit, together with some comment summing up the resoning for it so we don't forget why we are doing this here. Can you take another look?

[javanna]

could this be static?

[javanna]

LGTM thanks @cbuescher !