[test] use randomized runner in packaging tests #32109
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| /* | ||
| * Licensed to Elasticsearch under one or more contributor | ||
| * license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright | ||
| * ownership. Elasticsearch licenses this file to you under | ||
| * the Apache License, Version 2.0 (the "License"); you may | ||
| * not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.elasticsearch.packaging.test; | ||
|
|
||
| import com.carrotsearch.randomizedtesting.JUnit3MethodProvider; | ||
| import com.carrotsearch.randomizedtesting.RandomizedRunner; | ||
| import com.carrotsearch.randomizedtesting.annotations.TestMethodProviders; | ||
| import org.apache.commons.logging.Log; | ||
| import org.apache.commons.logging.LogFactory; | ||
| import org.junit.After; | ||
| import org.junit.Before; | ||
| import org.junit.Rule; | ||
| import org.junit.rules.TestName; | ||
| import org.junit.runner.RunWith; | ||
|
|
||
| @RunWith(RandomizedRunner.class) | ||
| @TestMethodProviders({ | ||
| JUnit3MethodProvider.class | ||
| }) | ||
| /** | ||
| * Class that all packaging test cases should inherit from. This makes working with the packaging tests more similar to what we're | ||
| * familiar with from {@link org.elasticsearch.test.ESTestCase} without having to apply its behavior that's not relevant here | ||
| */ | ||
| public abstract class PackagingTestCase { | ||
|
|
||
| protected final Log logger = LogFactory.getLog(getClass()); | ||
|
|
||
| @Rule | ||
| public final TestName testNameRule = new TestName(); | ||
|
|
||
| @Before | ||
| public void logTestNameBefore() { | ||
| logger.info("[" + testNameRule.getMethodName() + "]: before test"); | ||
|
There was a problem hiding this comment. Is this really necessary? Seems like it will produce a lot of noise. There was a problem hiding this comment. I think we want it because it'll be necessary to make sense of any other logging we do in the tests. The noise won't show up for people running it in the console because of how the vagrant tasks moderate their output - it'll only show up in CI because it runs with There was a problem hiding this comment. Do we really need a before and after? These are run completely sequentially, so the "before" of one test is the "after" of the previous. I'm just thinking of what the old output used to look like (a single line per test in most cases with "OK") compared to what we are moving to here (many lines per test, if I understand correctly). There was a problem hiding this comment. Makes sense, I removed the after test logging |
||
| } | ||
|
|
||
| @After | ||
| public void logTestnameAfter() { | ||
| logger.info("[" + testNameRule.getMethodName() + "]: after test"); | ||
| } | ||
|
|
||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for creating this class and replicating some of the stuff in ESTestCase instead of using it directly is that there are some things that ESTestCase does (e.g. bootstrap checks) that don't play well with how we run these tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we extend
ESTestCaseand disable what we don't need or doesn't work ?Some things might still be good to have here, I'm thinking about especially the reproduction line,
but I guess that wouldn't really work either because of how the order of test methods is inter-dependent.
What is the advantage o f having ti like that ? Would it make sense to bring everything that needs to run in order under a single method ? Then we could run in truly random fashion and perhaps keep more functionality from the test framework. As it is I'm not fully convinced that it's worth adding it just to throw out most of what it does and without really making use of any randomness in these tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have made this more clear - the real purpose of doing this is so that these tests will read similarly to the other junit tests wrt method names and such. There isn't as far as I can tell a way to configure that with any vanilla junit runners
There is no randomization here because the method ordering matters. The order of the test classes (not methods) could be randomized, but I don't think randomized runner does that and I didn't figure out a way to have it handle the suite itself rather than using
@RunWith(Suite.class)I spent some time on making it work with ESTestCase a while ago the first time I tried this and disabling/working around/making configurable the things in there that don't play nice with these tests was nontrivial as I remember, and I'm not sure I was really successful. It seemed much more straightforward to do it with its own parent test case
Currently we get a repro line from the vagrant test plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, using ESTestCase means bringing in elasticsearch as a dependency, which doesn't really make sense from a packaging test perspective (we won't make use of most of the test utilities provided there). We currently have elasticsearch as a dependency for the rest test runner, but it would be good to split that out so we can avoid the dependencies that ES pulls in (and can conflict with things we would like to do in the rest test runner, eg using the rest client).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could look at
GradleUnitTestCaseit does the same by pulling int the randomized runner only.What I was wondering about w.r.t order is that if it really makes sense to have it fixed. If all we are doing is going trough methods sequentially what advantage does it bring to have them in separate methods ? Maybe better error reporting ? Should we keep the randomized method order and make sure it actually works like that? I'm not saying we need to change it just looking to understand the implications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right, the benefit of the method ordering is mostly just to make it a little easier to read where something went wrong. And to separate out parts that aren't directly related to each other so it doesn't end up as just one very long test
I'd ideally like to randomize them and reinstall the distribution before each one so it starts from a predictable state (and some of them definitely can work like this already) but it would add too much runtime to an already very long test suite