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.

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

[test] remove Streamable serde assertions #29307

Merged

Conversation

andyb-elastic
Copy link
Contributor

@andyb-elastic andyb-elastic commented Mar 30, 2018

Removes a set of assertions in the test framework that verified that
Streamable objects could be serialized and deserialized across different
versions. When this was discussed the consensus was that this approach
has not caught many bugs in a long time and that serialization testing of
objects was best left to their respective unit and integration tests.

This commit also removes a transport interceptor that was used in
ESIntegTestCase tests to make these assertions about objects coming in
or off the wire.

Removes a set of assertions in the test framework that verified that
Streamable objects could be serialized and deserialized across different
versions. When this was discussed the consensus was that this approach
has not caught many bugs in a long time and that serialization testing of
objects was best left to their respective unit and integration tests.

This commit also removes a transport interceptor that was used in
ESIntegTestCase tests to make these assertions about objects coming in
or off the wire.
@andyb-elastic andyb-elastic added >test Issues or PRs that are addressing/adding tests review :Delivery/Build Build or test infrastructure v7.0.0 v6.3.0 labels Mar 30, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra


public static void assertSearchHit(SearchHit searchHit, Matcher<SearchHit> matcher) {
assertThat(searchHit, matcher);
assertVersionSerializable(searchHit);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method was inlined since it was now just passing its arguments to assertThat

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense 🌮

Copy link
Contributor

@talevy talevy left a comment

Choose a reason for hiding this comment

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

LGTM


public static void assertSearchHit(SearchHit searchHit, Matcher<SearchHit> matcher) {
assertThat(searchHit, matcher);
assertVersionSerializable(searchHit);
Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense 🌮

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

❤️ much less weird stuff going on.

@andyb-elastic andyb-elastic merged commit b7e6fb9 into elastic:master Mar 30, 2018
@andyb-elastic
Copy link
Contributor Author

Thanks guys

andyb-elastic added a commit that referenced this pull request Apr 2, 2018
Removes a set of assertions in the test framework that verified that
Streamable objects could be serialized and deserialized across different
versions. When this was discussed the consensus was that this approach
has not caught many bugs in a long time and that serialization testing of
objects was best left to their respective unit and integration tests.

This commit also removes a transport interceptor that was used in
ESIntegTestCase tests to make these assertions about objects coming in
or off the wire.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 3, 2018
* master: (80 commits)
  Remove HTTP max content length leniency (elastic#29337)
  Begin moving XContent to a separate lib/artifact (elastic#29300)
  Java versions for ci (elastic#29320)
  Minor cleanup in the InternalEngine (elastic#29241)
  Clarify expectations of false positives/negatives (elastic#27964)
  Update docs on vertex ordering (elastic#27963)
  Revert "REST high-level client: add support for Indices Update Settings API (elastic#28892)" (elastic#29323)
  [test] remove Streamable serde assertions (elastic#29307)
  Improve query string docs (elastic#28882)
  fix query string example for boolean query (elastic#28881)
  Resolve unchecked cast warnings introduced with elastic#28892
  REST high-level client: add support for Indices Update Settings API (elastic#28892)
  Search: Validate script query is run with a single script (elastic#29304)
  [DOCS] Added info on WGS-84. Closes issue elastic#3590 (elastic#29305)
  Increase timeout on Netty client latch for tests
  Build: Use branch specific refspec sysprop for bwc builds (elastic#29299)
  TEST: trim unsafe commits before opening engine
  Move trimming unsafe commits from engine ctor to store (elastic#29260)
  Fix incorrect geohash for lat 90, lon 180 (elastic#29256)
  Do not load global state when deleting a snapshot (elastic#29278)
  ...
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team >test Issues or PRs that are addressing/adding tests v6.3.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants