-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Add support for switching distribution for all integration tests #30874
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
Changes from 10 commits
ed0f95c
677cce5
73fa278
3364a43
9e457bd
fef4e22
e9579aa
c17a46d
ae9d375
2b4d2ca
8631413
7ba9499
e0fa121
cc454ec
b9faa69
80ffc38
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,7 @@ task setupSeedNodeAndUnicastHostsFile(type: DefaultTask) { | |
// setup the initial cluster with one node that will serve as the seed node | ||
// for unicast discovery | ||
ClusterConfiguration config = new ClusterConfiguration(project) | ||
config.distribution = 'integ-test-zip' | ||
config.distribution = System.getProperty('tests.distribution', 'integ-test-zip') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we need this line at all given your change to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would take precedence and override that. So it would be set back to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am concerned this is fragile. I don't want to see this logic copied around to other projects. Can you please create an issue for making creating additional cluster fixtures easier, so that the default can be set by PluginBuildPlugin for this project and reused the we create this cluster? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ++ on this being fragile. Fixing it in a followup is fine with me though. |
||
config.clusterName = 'discovery-file-test-cluster' | ||
List<NodeInfo> nodes = ClusterFormationTasks.setup(project, 'initialCluster', setupSeedNodeAndUnicastHostsFile, config) | ||
File srcUnicastHostsFile = file('build/cluster/unicast_hosts.txt') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,8 +30,12 @@ dependencies { | |
compileOnly project(':modules:lang-painless') | ||
} | ||
|
||
integTestCluster { | ||
distribution = 'zip' | ||
if (System.getProperty('tests.distribution') == null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems a bit complicated to add to every project. I'm not 100% sure we need to support There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It'd be fairly simple to fail the build if someone sets There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might be cleaner to move the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. setting distro There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that is generally useful. Plugins and modules are the projects which use integ-test-zip, and you can already cd into the plugins or modules dir and run tests from there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. I don't think we should support setting |
||
integTestCluster.distribution = 'oss-zip' | ||
} | ||
|
||
if (integTestCluster.distribution == 'integ-test-zip') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can remove this line because we don't support setting the setting to this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can add something to cluster formation tasks to make sure we don't set the system property to |
||
integTest.enabled = false | ||
} | ||
|
||
test.enabled = false |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,9 @@ subprojects { Project subproj -> | |
subproj.tasks.withType(RestIntegTestTask) { | ||
subproj.extensions.configure("${it.name}Cluster") { cluster -> | ||
cluster.distribution = System.getProperty('tests.distribution', 'oss-zip') | ||
if (cluster.distribution == 'integ-test-zip') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we're better off checking if the system property is |
||
throw new Exception("tests.distribution=integ.-test-zip is not supported") | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,13 +26,22 @@ | |
import org.elasticsearch.test.NotEqualMessageBuilder; | ||
|
||
import java.io.IOException; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Optional; | ||
import java.util.regex.Pattern; | ||
import java.util.stream.Collectors; | ||
import java.util.stream.Stream; | ||
|
||
import static org.elasticsearch.test.hamcrest.RegexMatcher.matches; | ||
import static org.hamcrest.Matchers.empty; | ||
import static org.hamcrest.Matchers.equalTo; | ||
import static org.hamcrest.Matchers.instanceOf; | ||
import static org.hamcrest.Matchers.is; | ||
import static org.hamcrest.Matchers.not; | ||
import static org.junit.Assert.assertNotNull; | ||
import static org.junit.Assert.assertThat; | ||
import static org.junit.Assert.assertTrue; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These look like leftovers. |
||
|
||
/** | ||
* Represents a match assert section: | ||
|
@@ -81,7 +90,36 @@ protected void doAssert(Object actualValue, Object expectedValue) { | |
} | ||
} | ||
|
||
if (expectedValue.equals(actualValue) == false) { | ||
// add support for matching objects ({a:b}) against list of objects ([ {a:b, c:d} ]) | ||
if(expectedValue instanceof Map && actualValue instanceof List) { | ||
Map<String, Object> expectedMap = (Map<String, Object>) expectedValue; | ||
List<Object> actualList = (List<Object>) actualValue; | ||
assertTrue( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The semantics seemed to fit with me, but now that I think of it, |
||
getField() + " was expected to be a list with Map but it's " + actualValue, | ||
actualList.stream() | ||
.filter((each) -> each instanceof Map) | ||
.findAny() | ||
.isPresent() | ||
); | ||
|
||
List<Map<String, Object>> actualValues = actualList.stream() | ||
.filter(each -> each instanceof Map) | ||
.map((each -> (Map<String, Object>) each)) | ||
.filter(each -> each.keySet().containsAll(expectedMap.keySet())) | ||
.collect(Collectors.toList()); | ||
assertThat( | ||
getField() + " expected to be a list with at least one object that has keys: " + | ||
expectedMap.keySet() + " but it was " + actualList, | ||
actualValues, | ||
is(not(empty())) | ||
); | ||
assertTrue( | ||
getField() + " expected to be a list with at least on object that matches " + expectedMap + | ||
" but was " + actualValues, | ||
actualValues.stream() | ||
.anyMatch(each -> each.entrySet().containsAll(expectedMap.entrySet())) | ||
); | ||
} else if (expectedValue.equals(actualValue) == false) { | ||
NotEqualMessageBuilder message = new NotEqualMessageBuilder(); | ||
message.compare(getField(), actualValue, expectedValue); | ||
throw new AssertionError(getField() + " didn't match expected value:\n" + message); | ||
|
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.
If using a non integ-test-zip distribution, modules need to not be installed at all. This looks like it allows installing them over what the distribution installs, which could create craziness. I think we need to skip setting up install module tasks if the distribution is not integ-test-zip.