Adds abstract test classes for serialisation by colings86 · Pull Request #22281 · elastic/elasticsearch

colings86 · 2016-12-20T11:53:57Z

This adds test classes that can be used to test the wire serialisation and (optionally) the XContent serialisation of objects that implement Streamable/Writeable and ToXContent.

These test classes will enable classes such as InternalAggregation (or at least its implementations) to be tested in a consistent way when is comes to testing serialisation.

s1monw

I really wonder if we can have at least one user of those tests?

s1monw · 2016-12-20T11:58:51Z

test/framework/src/main/java/org/elasticsearch/test/AbstractSerializingTestCase.java

this header is wrong

s1monw · 2016-12-20T12:01:08Z

test/framework/src/main/java/org/elasticsearch/test/AbstractStreamableTestCase.java

wrong header

s1monw · 2016-12-20T12:01:52Z

test/framework/src/main/java/org/elasticsearch/test/AbstractStreamableTestCase.java

why don't you make it random ie instead of this constant use randomIntBetween(1, 20)

s1monw · 2016-12-20T12:05:43Z

test/framework/src/main/java/org/elasticsearch/test/AbstractStreamableTestCase.java

will this work for all serialization classes? I mean queries need to be registered etc. I think we have to add all the ones we need for the subclass. So maybe add a factory method we can override?

s1monw · 2016-12-20T12:06:02Z

test/framework/src/main/java/org/elasticsearch/test/AbstractStreamableXContentTestCase.java

you should run gradle precommit

s1monw · 2016-12-20T12:06:29Z

test/framework/src/main/java/org/elasticsearch/test/AbstractWireSerializingTestCase.java

same comment as above

jpountz

Agreed with Simon it would help to have one user of this class. Also some protected abstract methods would benefit from having javadocs.

jpountz · 2016-12-20T14:04:50Z

test/framework/src/main/java/org/elasticsearch/test/AbstractStreamableTestCase.java

maybe use assertEquals instead to have better error messages

jpountz · 2016-12-20T14:07:14Z

test/framework/src/main/java/org/elasticsearch/test/AbstractStreamableXContentTestCase.java

can the docs give an example when it is useful to do so?

I copied this from AbstractQueryTestCase but actually I see that it's never used there. I'm not sure when this would be useful and I think we should avoid cases where the order of fields in a JSON object matters since keys in JSON objects are not guaranteed to be ordered so I'm going to remove this

colings86 · 2016-12-20T15:42:30Z

@s1monw @jpountz I pushed commits addressing your comments. I have implemented InternalMinTests which uses AbstractWireSerializingTestCase. I plan in a follow up PR to convert AbstractQueryTestCase and BaseAggregationTestCase to extend AbstractSerializingTestCase. Not sure if it's worth keeping the Streamable versions of the test cases since we want to move everything to use Writeable anyway?

colings86 · 2016-12-20T15:49:23Z

Also worth noting that because the test cases rely on hashCode and equals being implemented on the test class I had to add implementations to InternalAggregation and InternalNumericMetricAggregation which both declare methods that their subcleasses need to implement. In order to keep this PR mamageable and not require that I implemented hashCode and equals for all InternalAggregations subclasses I added a default implementation of those declared methods which use object identity, as well as adding a norelease comment to them stating that they should be made abstract when all the InternalAggregation subclasses implement the methods

jpountz

I left a comment about the way equals/hashcode are implemented, otherwise LGTM.

jpountz · 2016-12-20T17:27:17Z

core/src/main/java/org/elasticsearch/search/aggregations/InternalAggregation.java

Can you add javadocs?

jpountz · 2016-12-20T17:28:41Z

...in/java/org/elasticsearch/search/aggregations/metrics/InternalNumericMetricsAggregation.java

I don't like having both doEquals and innerEquals. Maybe just override equals to also include the format and still expect sub classes to implement doEquals? (Same for the hashcode)

This adds test classes that can be used to test the wire serialisation and (optionally) the XContent serialisation of objects that implement Streamable/Writeable and ToXContent. These test classes will enable classes sich as InternalAggregation (or at least its implementations) to be tested in a consistent way when is comes to testsing serialisation.

colings86 · 2016-12-21T13:19:20Z

@jpountz I pushed a new commit addressing your latest comments. Could you give this another look?

jpountz

LGTM

* master: (22 commits) Support negative numbers in writeVLong (elastic#22314) UnicastZenPing's PingingRound should prevent opening connections after being closed Add task to clean idea build directory. Make cleanIdea task invoke it. add trace logging to UnicastZenPingTests.testResolveReuseExistingNodeConnections Adds ingest processor headers to exception for unknown processor. (elastic#22315) Remove much ceremony from parsing client yaml test suites (elastic#22311) Support numeric bounds with decimal parts for long/integer/short/byte datatypes (elastic#21972) inner hits: Don't inline inner hits if the query the inner hits is inlined into can't resolve mappings and ignore_unmapped has been set to true Fix stackoverflow error on InternalNumericMetricAggregation Date detection should not rely on a hardcoded set of characters. (elastic#22171) `value_type` is useful regardless of scripting. (elastic#22160) Improve concurrency of ShardCoreKeyMap. (elastic#22316) fixed jdocs and removed already fixed norelease Adds abstract test classes for serialisation (elastic#22281) Introduce translog no-op Provide helpful error message if a plugin exists Clear static variable after suite Repeated language analyzers (elastic#22240) Restore deprecation warning for invalid match_mapping_type values (elastic#22304) Make `-0` compare less than `+0` consistently. (elastic#22173) ...

colings86 added review >test Issues or PRs that are addressing/adding tests v5.1.2 v5.2.0 v6.0.0-alpha1 labels Dec 20, 2016

colings86 self-assigned this Dec 20, 2016

colings86 requested review from jpountz and martijnvg December 20, 2016 11:53

s1monw suggested changes Dec 20, 2016

View reviewed changes

jpountz reviewed Dec 20, 2016

View reviewed changes

martijnvg mentioned this pull request Dec 21, 2016

Change Namespace for Stored Script to Only Use Id #22206

Merged

colings86 added 3 commits December 21, 2016 13:05

Review comments

d372a2f

more review comment changes

deeff6e

jpountz approved these changes Dec 21, 2016

View reviewed changes

colings86 merged commit 06576ed into elastic:master Dec 22, 2016

colings86 removed v5.1.2 v5.2.0 labels Dec 22, 2016

colings86 deleted the test/addAbstractSerializingTestCase branch December 22, 2016 10:49

imotov mentioned this pull request Feb 1, 2017

Expand AbstractSerializingTestCase and AbstractWireSerializingTestCase to test diff serialization #22906

Merged

Conversation

colings86 commented Dec 20, 2016

Uh oh!

s1monw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colings86 commented Dec 20, 2016

Uh oh!

colings86 commented Dec 20, 2016

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colings86 commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

colings86 commented Dec 21, 2016 •

edited

Loading