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

Increase coverage in SearchSortValuesTests #36597

Merged
merged 1 commit into from
Dec 14, 2018
Merged

Increase coverage in SearchSortValuesTests #36597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 0 additions & 1 deletion server/src/main/java/org/elasticsearch/search/SearchSortValues.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.common.xcontent.ToXContent.Params;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/search/SearchHitTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public static SearchHit createTestItem(XContentType xContentType, boolean withOp
hit.version(randomLong());
}
if (randomBoolean()) {
hit.sortValues(SearchSortValuesTests.createTestItem());
hit.sortValues(SearchSortValuesTests.createTestItem(xContentType, transportSerialization));
}
if (randomBoolean()) {
int size = randomIntBetween(0, 5);
Expand Down
150 changes: 78 additions & 72 deletions server/src/test/java/org/elasticsearch/search/SearchSortValuesTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,108 +19,114 @@

package org.elasticsearch.search;

import org.apache.lucene.util.BytesRef;
import org.elasticsearch.Version;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.stream.BytesStreamOutput;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.common.lucene.LuceneTests;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.common.xcontent.json.JsonXContent;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.AbstractSerializingTestCase;
import org.elasticsearch.test.RandomObjects;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.function.Supplier;

import static org.elasticsearch.common.xcontent.XContentHelper.toXContent;
import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent;

public class SearchSortValuesTests extends ESTestCase {

public static SearchSortValues createTestItem() {
List<Supplier<Object>> valueSuppliers = new ArrayList<>();
// this should reflect all values that are allowed to go through the transport layer
valueSuppliers.add(() -> null);
valueSuppliers.add(ESTestCase::randomInt);
valueSuppliers.add(ESTestCase::randomLong);
valueSuppliers.add(ESTestCase::randomDouble);
valueSuppliers.add(ESTestCase::randomFloat);
valueSuppliers.add(ESTestCase::randomByte);
valueSuppliers.add(ESTestCase::randomShort);
valueSuppliers.add(ESTestCase::randomBoolean);
valueSuppliers.add(() -> frequently() ? randomAlphaOfLengthBetween(1, 30) : randomRealisticUnicodeOfCodepointLength(30));
public class SearchSortValuesTests extends AbstractSerializingTestCase<SearchSortValues> {

public static SearchSortValues createTestItem(XContentType xContentType, boolean transportSerialization) {
int size = randomIntBetween(1, 20);
Object[] values = new Object[size];
DocValueFormat[] sortValueFormats = new DocValueFormat[size];
for (int i = 0; i < size; i++) {
Supplier<Object> supplier = randomFrom(valueSuppliers);
values[i] = supplier.get();
Object sortValue = randomSortValue(xContentType, transportSerialization);
values[i] = sortValue;
//make sure that for BytesRef, we provide a specific doc value format that overrides format(BytesRef)
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity: why is that important? Is it easy enough to explain to add to the comment without making it too long?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand the question? What is too long? This is a tricky bit of the sort values, that they get formatted when doc value formats are provided (which happens whenever the class is used on the server-side).

sortValueFormats[i] = sortValue instanceof BytesRef ? DocValueFormat.RAW : randomDocValueFormat();
}
return new SearchSortValues(values);
return new SearchSortValues(values, sortValueFormats);
}

private static Object randomSortValue(XContentType xContentType, boolean transportSerialization) {
Object randomSortValue = LuceneTests.randomSortValue();
//to simplify things, we directly serialize what we expect we would parse back when testing xcontent serialization
return transportSerialization ? randomSortValue : RandomObjects.getExpectedParsedValue(xContentType, randomSortValue);
}

public void testFromXContent() throws IOException {
SearchSortValues sortValues = createTestItem();
XContentType xcontentType = randomFrom(XContentType.values());
boolean humanReadable = randomBoolean();
BytesReference originalBytes = toShuffledXContent(sortValues, xcontentType, ToXContent.EMPTY_PARAMS, humanReadable);
private static DocValueFormat randomDocValueFormat() {
return randomFrom(DocValueFormat.BOOLEAN, DocValueFormat.RAW, DocValueFormat.IP, DocValueFormat.BINARY, DocValueFormat.GEOHASH);
}

SearchSortValues parsed;
try (XContentParser parser = createParser(xcontentType.xContent(), originalBytes)) {
parser.nextToken(); // skip to the elements start array token, fromXContent advances from there if called
parser.nextToken();
parser.nextToken();
parsed = SearchSortValues.fromXContent(parser);
parser.nextToken();
assertEquals(XContentParser.Token.END_OBJECT, parser.currentToken());
assertNull(parser.nextToken());
}
assertToXContentEquivalent(originalBytes, toXContent(parsed, xcontentType, humanReadable), xcontentType);
@Override
protected SearchSortValues doParseInstance(XContentParser parser) throws IOException {
parser.nextToken(); // skip to the elements start array token, fromXContent advances from there if called
parser.nextToken();
parser.nextToken();
SearchSortValues searchSortValues = SearchSortValues.fromXContent(parser);
parser.nextToken();
assertEquals(XContentParser.Token.END_OBJECT, parser.currentToken());
assertNull(parser.nextToken());
return searchSortValues;
}

public void testToXContent() throws IOException {
SearchSortValues sortValues = new SearchSortValues(new Object[]{ 1, "foo", 3.0});
XContentBuilder builder = JsonXContent.contentBuilder();
builder.startObject();
sortValues.toXContent(builder, ToXContent.EMPTY_PARAMS);
builder.endObject();
assertEquals("{\"sort\":[1,\"foo\",3.0]}", Strings.toString(builder));
@Override
protected SearchSortValues createXContextTestInstance(XContentType xContentType) {
return createTestItem(xContentType, false);
}

/**
* Test equality and hashCode properties
*/
public void testEqualsAndHashcode() {
checkEqualsAndHashCode(createTestItem(), SearchSortValuesTests::copy, SearchSortValuesTests::mutate);
@Override
protected SearchSortValues createTestInstance() {
return createTestItem(randomFrom(XContentType.values()), true);
}

public void testSerialization() throws IOException {
SearchSortValues sortValues = createTestItem();
try (BytesStreamOutput output = new BytesStreamOutput()) {
sortValues.writeTo(output);
try (StreamInput in = output.bytes().streamInput()) {
SearchSortValues deserializedCopy = new SearchSortValues(in);
assertEquals(sortValues, deserializedCopy);
assertEquals(sortValues.hashCode(), deserializedCopy.hashCode());
assertNotSame(sortValues, deserializedCopy);
}
@Override
protected Writeable.Reader<SearchSortValues> instanceReader() {
return SearchSortValues::new;
}

@Override
protected String[] getShuffleFieldsExceptions() {
return new String[]{"sort"};
}

public void testToXContent() throws IOException {
{
SearchSortValues sortValues = new SearchSortValues(new Object[]{ 1, "foo", 3.0});
XContentBuilder builder = JsonXContent.contentBuilder();
builder.startObject();
sortValues.toXContent(builder, ToXContent.EMPTY_PARAMS);
builder.endObject();
assertEquals("{\"sort\":[1,\"foo\",3.0]}", Strings.toString(builder));
}
{
SearchSortValues sortValues = new SearchSortValues(new Object[0]);
XContentBuilder builder = JsonXContent.contentBuilder();
builder.startObject();
sortValues.toXContent(builder, ToXContent.EMPTY_PARAMS);
builder.endObject();
assertEquals("{}", Strings.toString(builder));
}
}

private static SearchSortValues mutate(SearchSortValues original) {
Object[] sortValues = original.sortValues();
@Override
protected SearchSortValues mutateInstance(SearchSortValues instance) {
Object[] sortValues = instance.sortValues();
if (sortValues.length == 0) {
return new SearchSortValues(new Object[] { 1 });
return createTestInstance();
}
return new SearchSortValues(Arrays.copyOf(sortValues, sortValues.length + 1));
if (randomBoolean()) {
return new SearchSortValues(new Object[0]);
Copy link
Member

Choose a reason for hiding this comment

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

this looks like a rare case, what is it for?

Copy link
Member Author

Choose a reason for hiding this comment

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

just checking if equals/hashcode work well in this corner-case too

}
Object[] values = Arrays.copyOf(sortValues, sortValues.length + 1);
values[sortValues.length] = randomSortValue(randomFrom(XContentType.values()), true);
return new SearchSortValues(values);
}

private static SearchSortValues copy(SearchSortValues original) {
return new SearchSortValues(Arrays.copyOf(original.sortValues(), original.sortValues().length));
@Override
protected SearchSortValues copyInstance(SearchSortValues instance, Version version) {
return new SearchSortValues(Arrays.copyOf(instance.sortValues(), instance.sortValues().length));
}
}
31 changes: 20 additions & 11 deletions test/framework/src/main/java/org/elasticsearch/test/AbstractSerializingTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,35 +24,44 @@
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;

import java.io.IOException;
import java.util.function.Predicate;

import static org.elasticsearch.test.AbstractXContentTestCase.xContentTester;

public abstract class AbstractSerializingTestCase<T extends ToXContent & Writeable> extends AbstractWireSerializingTestCase<T> {

/**
* Generic test that creates new instance from the test instance and checks
* both for equality and asserts equality on the two instances.
*/
public final void testFromXContent() throws IOException {
AbstractXContentTestCase.testFromXContent(
NUMBER_OF_TEST_RUNS,
this::createTestInstance,
supportsUnknownFields(),
getShuffleFieldsExceptions(),
getRandomFieldsExcludeFilter(),
this::createParser,
this::doParseInstance,
this::assertEqualInstances,
assertToXContentEquivalence(),
getToXContentParams());
xContentTester(this::createParser, this::createXContextTestInstance, getToXContentParams(), this::doParseInstance)
.numberOfTestRuns(NUMBER_OF_TEST_RUNS)
.supportsUnknownFields(supportsUnknownFields())
.shuffleFieldsExceptions(getShuffleFieldsExceptions())
.randomFieldsExcludeFilter(getRandomFieldsExcludeFilter())
.assertEqualsConsumer(this::assertEqualInstances)
.assertToXContentEquivalence(assertToXContentEquivalence())
.test();
}

/**
* Parses to a new instance using the provided {@link XContentParser}
*/
protected abstract T doParseInstance(XContentParser parser) throws IOException;

/**
* Creates a random instance to use in the xcontent tests.
* Override this method if the random instance that you build
* should be aware of the {@link XContentType} used in the test.
*/
protected T createXContextTestInstance(XContentType xContentType) {
return createTestInstance();
}

/**
* Indicates whether the parser supports unknown fields or not. In case it does, such behaviour will be tested by
* inserting random fields before parsing and checking that they don't make parsing fail.
Expand Down
100 changes: 53 additions & 47 deletions test/framework/src/main/java/org/elasticsearch/test/RandomObjects.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

import com.carrotsearch.randomizedtesting.generators.RandomPicks;
import com.carrotsearch.randomizedtesting.generators.RandomStrings;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.action.support.replication.ReplicationResponse.ShardInfo;
import org.elasticsearch.action.support.replication.ReplicationResponse.ShardInfo.Failure;
Expand Down Expand Up @@ -72,78 +71,85 @@ private RandomObjects() {
*/
public static Tuple<List<Object>, List<Object>> randomStoredFieldValues(Random random, XContentType xContentType) {
int numValues = randomIntBetween(random, 1, 5);
List<Object> originalValues = new ArrayList<>();
List<Object> expectedParsedValues = new ArrayList<>();
List<Object> originalValues = randomStoredFieldValues(random, numValues);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can also use LuceneTests#randomSortValue() here and get rif of the private helper alltogether? I see its slightly different in the way it e.g. draws strings, but then again we also use it in https://github.com/elastic/elasticsearch/pull/36597/files#diff-c5ebccd66a4f939ea845842b8cfea549R54 for the single-value case as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Sort values and stored fields are two different things. The differences are mainly around strings/byte arrays indeed. We could consider simplifying things in a follow-up but this PR does not make things worse, I made sure that we share the common bits around parsing and the difference between original and parsed value when they go through xcontent serialization.

List<Object> expectedParsedValues = new ArrayList<>(numValues);
for (Object originalValue : originalValues) {
expectedParsedValues.add(getExpectedParsedValue(xContentType, originalValue));
}
return Tuple.tuple(originalValues, expectedParsedValues);
}

private static List<Object> randomStoredFieldValues(Random random, int numValues) {
List<Object> values = new ArrayList<>(numValues);
int dataType = randomIntBetween(random, 0, 8);
for (int i = 0; i < numValues; i++) {
switch(dataType) {
case 0:
long randomLong = random.nextLong();
originalValues.add(randomLong);
expectedParsedValues.add(randomLong);
values.add(random.nextLong());
break;
case 1:
int randomInt = random.nextInt();
originalValues.add(randomInt);
expectedParsedValues.add(randomInt);
values.add(random.nextInt());
break;
case 2:
Short randomShort = (short) random.nextInt();
originalValues.add(randomShort);
expectedParsedValues.add(randomShort.intValue());
values.add((short) random.nextInt());
break;
case 3:
Byte randomByte = (byte)random.nextInt();
originalValues.add(randomByte);
expectedParsedValues.add(randomByte.intValue());
values.add((byte) random.nextInt());
break;
case 4:
double randomDouble = random.nextDouble();
originalValues.add(randomDouble);
expectedParsedValues.add(randomDouble);
values.add(random.nextDouble());
break;
case 5:
Float randomFloat = random.nextFloat();
originalValues.add(randomFloat);
if (xContentType == XContentType.CBOR) {
//with CBOR we get back a float
expectedParsedValues.add(randomFloat);
} else if (xContentType == XContentType.SMILE) {
//with SMILE we get back a double (this will change in Jackson 2.9 where it will return a Float)
expectedParsedValues.add(randomFloat.doubleValue());
} else {
//with JSON AND YAML we get back a double, but with float precision.
expectedParsedValues.add(Double.parseDouble(randomFloat.toString()));
}
values.add(random.nextFloat());
break;
case 6:
boolean randomBoolean = random.nextBoolean();
originalValues.add(randomBoolean);
expectedParsedValues.add(randomBoolean);
values.add(random.nextBoolean());
break;
case 7:
String randomString = random.nextBoolean() ? RandomStrings.randomAsciiLettersOfLengthBetween(random, 3, 10) :
randomUnicodeOfLengthBetween(random, 3, 10);
originalValues.add(randomString);
expectedParsedValues.add(randomString);
values.add(random.nextBoolean() ? RandomStrings.randomAsciiLettersOfLengthBetween(random, 3, 10) :
randomUnicodeOfLengthBetween(random, 3, 10));
break;
case 8:
byte[] randomBytes = RandomStrings.randomUnicodeOfLengthBetween(random, 10, 50).getBytes(StandardCharsets.UTF_8);
BytesArray randomBytesArray = new BytesArray(randomBytes);
originalValues.add(randomBytesArray);
if (xContentType == XContentType.JSON || xContentType == XContentType.YAML) {
//JSON and YAML write the base64 format
expectedParsedValues.add(Base64.getEncoder().encodeToString(randomBytes));
} else {
//SMILE and CBOR write the original bytes as they support binary format
expectedParsedValues.add(randomBytesArray);
}
values.add(new BytesArray(randomBytes));
break;
default:
throw new UnsupportedOperationException();
}
}
return Tuple.tuple(originalValues, expectedParsedValues);
return values;
}

/**
* Converts the provided field value to its corresponding expected value once printed out
* via {@link org.elasticsearch.common.xcontent.ToXContent#toXContent(XContentBuilder, ToXContent.Params)} and parsed back via
* {@link org.elasticsearch.common.xcontent.XContentParser#objectText()}.
* Generates values based on what can get printed out. Stored fields values are retrieved from lucene and converted via
Copy link
Member

Choose a reason for hiding this comment

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

nit: s/lucene/Lucene

Copy link
Member Author

@javanna javanna Dec 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We have plenty of references to either lucene or Lucene in our codebase comments. I am not sure that going for one or the other makes things more consistent.

* {@link org.elasticsearch.index.mapper.MappedFieldType#valueForDisplay(Object)} to either strings, numbers or booleans.
*/
public static Object getExpectedParsedValue(XContentType xContentType, Object value) {
if (value instanceof BytesArray) {
if (xContentType == XContentType.JSON || xContentType == XContentType.YAML) {
//JSON and YAML write the base64 format
return Base64.getEncoder().encodeToString(((BytesArray)value).toBytesRef().bytes);
}
}
if (value instanceof Float) {
if (xContentType == XContentType.SMILE) {
//with SMILE we get back a double (this will change in Jackson 2.9 where it will return a Float)
return ((Float)value).doubleValue();
} else {
//with JSON AND YAML we get back a double, but with float precision.
return Double.parseDouble(value.toString());
}
}
if (value instanceof Byte) {
return ((Byte)value).intValue();
}
if (value instanceof Short) {
return ((Short)value).intValue();
}
return value;
}

/**
Expand Down