Add MapperScriptTestCase #71322
Add MapperScriptTestCase #71322
Conversation
romseygeek
added
>test
|
Pinging @elastic/es-search (Team:Search) |
| @SuppressWarnings("unused") | ||
| public static final String[] PARAMETERS = {}; | ||
|
|
||
| public static DoubleFieldScript.Factory factory(Consumer<DoubleFieldScript> executor) { |
There was a problem hiding this comment.
This method, in combination with making emit public, makes writing tests so much easier,
There was a problem hiding this comment.
Would it help similarly if DoubleFieldScript implements an interface and tests didn't use the AbstractFieldScript infrastructure at all? Its kind of designed around incantations that work well in painless which isn't really a concern that the mapping infrastructure needs to have. Maybe it'd be easier to reason about with an interface and the tests just implementing that directly? I dunno.
There was a problem hiding this comment.
I looked at that, but this turns out to be simpler - the interface ends up basically duplicating all of the logic except construction.
There was a problem hiding this comment.
do we end up executing the script in these tests? I was wondering if instead of taking a consumer of the field script, this method should take a supplier of field script, then the test would subclass the script class directly... maybe you have already tried it though. Also maybe this could be one method in the base mapper script test class with a generic type?
There was a problem hiding this comment.
do we end up executing the script in these tests?
Yes - see for example testMultipleValues in LongScriptMapperTests
The reason I've done it this way is that the ceremony is all around construction, and returning a subclass doesn't help there because you still need to pass all the parameters to the super constructor. And you can't construct a generic class without knowing the type, so it can't be a method on the base test class.
There was a problem hiding this comment.
makes sense, can we then move these new static methods to their corresponding test classes, assuming they are only used there?
| return randomBoolean() ? randomDoubleBetween(-Double.MAX_VALUE, Double.MAX_VALUE, true) : randomFloat(); | ||
| } | ||
|
|
||
| public void testScriptAndPrecludedParameters() { |
There was a problem hiding this comment.
These were already on the long field tests but I think it's a good idea to have them here as well
There was a problem hiding this comment.
Should they be in MapperScriptTestCase then? Like a final test method? I think we have a reasonable way to get the type being tested, right?
There was a problem hiding this comment.
They only apply to double and long though. I'm sure there will be precluded params for date, keyword, etc, but they will be different. I may end up adding a getPrecludedParams method when I add the next implementations though.
There was a problem hiding this comment.
👍 to keeping them separate then. I get blind to copy-and-paste differences so I try to avoid them if possible. But, shrug.
You sound like my daughter. |
She sounds like mine :) |
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
left a couple of minor comments, LGTM otherwise
| IndexableField[] fields = doc.rootDoc().getFields("field"); | ||
| assertEquals(1, fields.length); | ||
| assertEquals("docValuesType=SORTED_NUMERIC<field:4>", fields[0].toString()); | ||
| } |
There was a problem hiding this comment.
could these three methods somehow be in the base test class, at least partially? what I am looking for is avoiding copy pasting when writing new tests, and possibly not forgetting to cover important scenarios.
| return (T) compileScript(script.getIdOrCode()); | ||
| } | ||
|
|
||
| public void testSerialization() throws IOException { |
There was a problem hiding this comment.
nit: call it testToXContent? I somehow think of serialization as the transport one, so I get confused with this naming.
| assertThat(e.getMessage(), containsString("Cannot define script on field with index:false and doc_values:false")); | ||
| } | ||
|
|
||
| public void testScriptErrorParameterRequiresScript() { |
When we added scripts to long and double mapped fields, we added tests for the general scripting infrastructure, and also specific tests for those two field types. This commit extracts those type-specific tests out into a new base test class that we can use when adding scripts to more field mappers.
When we added scripts to long and double mapped fields, we added tests for the general scripting infrastructure, and also specific tests for those two field types. This commit extracts those type-specific tests out into a new base test class that we can use when adding scripts to more field mappers. Co-authored-by: Alan Woodward <[email protected]>
5 participants
When we added scripts to
longanddoublemapped fields, we added testsfor the general scripting infrastructure, and also specific tests for those two
field types. This commit extracts those type-specific tests out into a new base
test class that we can use when adding scripts to more field mappers.