Adds abstract test classes for serialisation #22281
Conversation
colings86
added
review
>test
s1monw
left a comment
s1monw
left a comment
There was a problem hiding this comment.
I really wonder if we can have at least one user of those tests?
There was a problem hiding this comment.
why don't you make it random ie instead of this constant use randomIntBetween(1, 20)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
you should run gradle precommit
jpountz
left a comment
jpountz
left a comment
There was a problem hiding this comment.
Agreed with Simon it would help to have one user of this class. Also some protected abstract methods would benefit from having javadocs.
There was a problem hiding this comment.
maybe use assertEquals instead to have better error messages
There was a problem hiding this comment.
can the docs give an example when it is useful to do so?
There was a problem hiding this comment.
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
|
@s1monw @jpountz I pushed commits addressing your comments. I have implemented InternalMinTests which uses |
|
Also worth noting that because the test cases rely on |
jpountz
left a comment
jpountz
left a comment
There was a problem hiding this comment.
I left a comment about the way equals/hashcode are implemented, otherwise LGTM.
There was a problem hiding this comment.
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.
|
@jpountz I pushed a new commit addressing your latest comments. Could you give this another look? |
* 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) ...
3 participants
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.