-
Notifications
You must be signed in to change notification settings - Fork 25.6k
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 all 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 |
|---|---|---|
|
|
@@ -30,8 +30,8 @@ dependencies { | |
| compileOnly project(':modules:lang-painless') | ||
| } | ||
|
|
||
| integTestCluster { | ||
| distribution = 'zip' | ||
| if (System.getProperty('tests.distribution') == null) { | ||
|
Member
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
Member
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
Member
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
Contributor
Author
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
Member
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.
Member
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' | ||
| } | ||
|
|
||
| test.enabled = false | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| /* | ||
| * 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.test.rest.yaml.section; | ||
|
|
||
| import org.apache.logging.log4j.Logger; | ||
| import org.elasticsearch.common.collect.Tuple; | ||
| import org.elasticsearch.common.logging.Loggers; | ||
| import org.elasticsearch.common.xcontent.XContentLocation; | ||
| import org.elasticsearch.common.xcontent.XContentParser; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| import static org.hamcrest.Matchers.empty; | ||
| import static org.hamcrest.Matchers.is; | ||
| import static org.hamcrest.Matchers.not; | ||
| import static org.junit.Assert.assertThat; | ||
| import static org.junit.Assert.assertTrue; | ||
| import static org.junit.Assert.fail; | ||
|
|
||
| public class ContainsAssertion extends Assertion { | ||
| public static ContainsAssertion parse(XContentParser parser) throws IOException { | ||
| XContentLocation location = parser.getTokenLocation(); | ||
| Tuple<String,Object> stringObjectTuple = ParserUtils.parseTuple(parser); | ||
| return new ContainsAssertion(location, stringObjectTuple.v1(), stringObjectTuple.v2()); | ||
| } | ||
|
|
||
| private static final Logger logger = Loggers.getLogger(ContainsAssertion.class); | ||
|
|
||
| public ContainsAssertion(XContentLocation location, String field, Object expectedValue) { | ||
| super(location, field, expectedValue); | ||
| } | ||
|
|
||
| @Override | ||
| protected void doAssert(Object actualValue, Object expectedValue) { | ||
| // add support for matching objects ({a:b}) against list of objects ([ {a:b, c:d} ]) | ||
| if(expectedValue instanceof Map && actualValue instanceof List) { | ||
| logger.trace("assert that [{}] contains [{}]", actualValue, expectedValue); | ||
| Map<String, Object> expectedMap = (Map<String, Object>) expectedValue; | ||
| List<Object> actualList = (List<Object>) actualValue; | ||
| List<Map<String, Object>> actualValues = actualList.stream() | ||
| .filter(each -> each instanceof Map) | ||
|
Member
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 should fail the assertion if the thing we're testing isn't a map.
Contributor
Author
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. As implemented now, we can check that a list has an object that has all the keys at the specific values. In theory we could have a list having a mix of lists and objects. The filter only rules out anything that's not a map at this point when looking for
Member
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 see! Got it. |
||
| .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 { | ||
| fail("'contains' only supports checking an object against a list of objects"); | ||
| } | ||
| } | ||
| } | ||
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'm not sure we need this line at all given your change to
PluginBuildPlugin.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.
This would take precedence and override that. So it would be set back to
integ-test-zipregardless of the property.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 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 comment
The 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.