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

Fix edge case in PutMappingRequestTests #37665

Merged
merged 1 commit into from
Jan 23, 2019
Merged

Fix edge case in PutMappingRequestTests #37665

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
23 changes: 15 additions & 8 deletions ...est-high-level/src/test/java/org/elasticsearch/client/indices/PutMappingRequestTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,14 @@

package org.elasticsearch.client.indices;

import org.apache.lucene.util.LuceneTestCase.AwaitsFix;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.RandomCreateIndexGenerator;
import org.elasticsearch.test.AbstractXContentTestCase;

import java.io.IOException;
import java.util.Map;

@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/37654")
public class PutMappingRequestTests extends AbstractXContentTestCase<PutMappingRequest> {

@Override
Expand All @@ -47,7 +46,10 @@ protected PutMappingRequest createTestInstance() {
@Override
protected PutMappingRequest doParseInstance(XContentParser parser) throws IOException {
PutMappingRequest request = new PutMappingRequest();
request.source(parser.map());
Map<String, Object> map = parser.map();
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering -- would it be a bit unintuitive for a user if they specified a non-null source, but then when going to inspect it, it is null? We could instead only modify the tests (perhaps always specifying a non-null source in createTestInstance, and adding a separate test to verify the null source behavior).

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I understand this parsing code only lives in the test? With the new client-side request that differ from the server side one (which eventually can be fully internal adfter the transport client removal) the parsing is done on the server side, so the user will always see their null-source still. Its only replaced by an empty object when sent.
To the contrary, when working on this fix got to the point where I think that we are overtesting these simple client-side POJOs (they are more or less structs with a toXContent and some validation methods). Having to dig around in AbstractXContentTestCase made me think which parts of it we really need. I'd really be in favour of making these simple ESTestCases, skip most of the randomization and only check all the possible code paths in the xContent output.

Copy link
Member Author

Choose a reason for hiding this comment

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

always specifying a non-null source in createTestInstance

Adding to this, I think the occasional null source is the only useful variant in this test at the moment since it checks that toXContent doesn't blow up on it.

Copy link
Contributor

@jtibshirani jtibshirani Jan 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ah you're right, my original comment doesn't actually make sense. I hadn't yet had my morning coffee :)

I also like the idea of making this a simple ESTestCase -- are you planning to do this in the current PR? I'm also happy to merge this now, and improve this test in a separate PR if you'd prefer that.

Copy link
Member Author

Choose a reason for hiding this comment

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

are you planning to do this in the current PR

No, I wasn't but I can sure do this as a follow up.

if (map.isEmpty() == false) {
request.source(map);
}
return request;
}

Expand All @@ -58,11 +60,16 @@ protected boolean supportsUnknownFields() {

@Override
protected void assertEqualInstances(PutMappingRequest expected, PutMappingRequest actual) {
try (XContentParser expectedJson = createParser(expected.xContentType().xContent(), expected.source());
XContentParser actualJson = createParser(actual.xContentType().xContent(), actual.source())) {
assertEquals(expectedJson.mapOrdered(), actualJson.mapOrdered());
} catch (IOException e) {
throw new RuntimeException(e);
if (actual.source() != null) {
try (XContentParser expectedJson = createParser(expected.xContentType().xContent(), expected.source());
XContentParser actualJson = createParser(actual.xContentType().xContent(), actual.source())) {
assertEquals(expectedJson.mapOrdered(), actualJson.mapOrdered());
} catch (IOException e) {
throw new RuntimeException(e);
}
} else {
// if the original `source` is null, the parsed source should be so too
assertNull(expected.source());
}
}
}