Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[test] use randomized runner in packaging tests #32109

Merged
merged 4 commits into from
Jul 18, 2018
Merged

[test] use randomized runner in packaging tests #32109

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a569a8c
[test] use randomized runner in packaging tests
andyb-elastic Jul 16, 2018
568b7dd
use randomizedtesting directly
andyb-elastic Jul 17, 2018
5b7397a
log only before test, not after
andyb-elastic Jul 17, 2018
9ec1584
checkstyle fixes
andyb-elastic Jul 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions qa/vagrant/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,15 @@ plugins {

dependencies {
compile "junit:junit:${versions.junit}"
compile "org.hamcrest:hamcrest-core:${versions.hamcrest}"
compile "org.hamcrest:hamcrest-library:${versions.hamcrest}"
compile "org.hamcrest:hamcrest-all:${versions.hamcrest}"

compile "org.apache.httpcomponents:httpcore:${versions.httpcore}"
compile "org.apache.httpcomponents:httpclient:${versions.httpclient}"
compile "org.apache.httpcomponents:fluent-hc:${versions.httpclient}"
compile "commons-codec:commons-codec:${versions.commonscodec}"
compile "commons-logging:commons-logging:${versions.commonslogging}"

compile "org.elasticsearch.test:framework:${version}"
Copy link
Member

Choose a reason for hiding this comment

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

I think you could pull in randomized runner on its own? Additionally, there is a JUnit3MethodProvider in randomized runner, can we use that instead of the lucene one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, I changed it back to use randomizedtesting instead of the framework - I'd added it when I was trying to make it work with ESTestCase

compile project(':libs:core')

// pulls in the jar built by this project and its dependencies
Expand Down Expand Up @@ -85,11 +85,15 @@ tasks.thirdPartyAudit.excludes = [
'org.apache.avalon.framework.logger.Logger',
'org.apache.log.Hierarchy',
'org.apache.log.Logger',
'org.apache.log4j.Category',
'org.apache.log4j.Level',
'org.apache.log4j.Logger',
'org.apache.log4j.Priority',
//commons-logging provided dependencies
'javax.servlet.ServletContextEvent',
'javax.servlet.ServletContextListener'
'javax.servlet.ServletContextListener',
// brought in from test framework
'org.apache.tools.ant.BuildException',
'org.apache.tools.ant.DirectoryScanner',
'org.apache.tools.ant.Task',
'org.apache.tools.ant.types.FileSet',
'org.easymock.EasyMock',
'org.easymock.IArgumentMatcher',
'org.jmock.core.Constraint'
]
16 changes: 3 additions & 13 deletions qa/vagrant/src/main/java/org/elasticsearch/packaging/test/ArchiveTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.elasticsearch.packaging.test;

import com.carrotsearch.randomizedtesting.annotations.TestCaseOrdering;
import org.apache.http.client.fluent.Request;
import org.elasticsearch.packaging.util.Archives;
import org.elasticsearch.packaging.util.Platforms;
Expand All @@ -27,9 +28,6 @@
import org.elasticsearch.packaging.util.Shell.Result;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.FixMethodOrder;
import org.junit.Test;
import org.junit.runners.MethodSorters;

import org.elasticsearch.packaging.util.Distribution;
import org.elasticsearch.packaging.util.Installation;
Expand Down Expand Up @@ -67,8 +65,8 @@
* Tests that apply to the archive distributions (tar, zip). To add a case for a distribution, subclass and
* override {@link ArchiveTestCase#distribution()}. These tests should be the same across all archive distributions
*/
@FixMethodOrder(MethodSorters.NAME_ASCENDING)
public abstract class ArchiveTestCase {
@TestCaseOrdering(TestCaseOrdering.AlphabeticOrder.class)
public abstract class ArchiveTestCase extends PackagingTestCase {

private static Installation installation;

Expand All @@ -86,13 +84,11 @@ public void onlyCompatibleDistributions() {
assumeTrue("only compatible distributions", distribution().packaging.compatible);
}

@Test
public void test10Install() {
installation = installArchive(distribution());
verifyArchiveInstallation(installation, distribution());
}

@Test
public void test20PluginsListWithNoPlugins() {
assumeThat(installation, is(notNullValue()));

Expand All @@ -103,7 +99,6 @@ public void test20PluginsListWithNoPlugins() {
assertThat(r.stdout, isEmptyString());
}

@Test
public void test30AbortWhenJavaMissing() {
assumeThat(installation, is(notNullValue()));

Expand Down Expand Up @@ -146,7 +141,6 @@ public void test30AbortWhenJavaMissing() {
});
}

@Test
public void test40CreateKeystoreManually() {
assumeThat(installation, is(notNullValue()));

Expand Down Expand Up @@ -180,7 +174,6 @@ public void test40CreateKeystoreManually() {
});
}

@Test
public void test50StartAndStop() throws IOException {
assumeThat(installation, is(notNullValue()));

Expand All @@ -198,7 +191,6 @@ public void test50StartAndStop() throws IOException {
Archives.stopElasticsearch(installation);
}

@Test
public void test60AutoCreateKeystore() {
assumeThat(installation, is(notNullValue()));

Expand All @@ -218,7 +210,6 @@ public void test60AutoCreateKeystore() {
});
}

@Test
public void test70CustomPathConfAndJvmOptions() throws IOException {
assumeThat(installation, is(notNullValue()));

Expand Down Expand Up @@ -268,7 +259,6 @@ public void test70CustomPathConfAndJvmOptions() throws IOException {
}
}

@Test
public void test80RelativePathConf() throws IOException {
assumeThat(installation, is(notNullValue()));

Expand Down
58 changes: 58 additions & 0 deletions qa/vagrant/src/main/java/org/elasticsearch/packaging/test/PackagingTestCase.java
View file
Open in desktop
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.RandomizedRunner;
import com.carrotsearch.randomizedtesting.annotations.TestMethodProviders;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.apache.lucene.util.LuceneJUnit3MethodProvider;
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({
LuceneJUnit3MethodProvider.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();
Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we extend ESTestCase and 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.

Copy link
Contributor Author

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

Copy link
Member

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

You could look at GradleUnitTestCase it 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.

Copy link
Contributor Author

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


@Before
public void logTestNameBefore() {
logger.info("[" + testNameRule.getMethodName() + "]: before test");
Copy link
Member

Choose a reason for hiding this comment

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

Is this really necessary? Seems like it will produce a lot of noise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 --console plain --info

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I removed the after test logging

}

@After
public void logTestnameAfter() {
logger.info("[" + testNameRule.getMethodName() + "]: after test");
}

}