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

Migrate some *ResponseTests to AbstractStreamableXContentTestCase #28749

Merged

Conversation

javanna
Copy link
Member

@javanna javanna commented Feb 20, 2018

This allows us to save a bit of code, but also adds more coverage as it tests serialization which was missing in some of the existing tests. Also it requires implementing equals/hashcode and we get the corresponding tests for them for free from the base test class.

@javanna javanna added >test Issues or PRs that are addressing/adding tests review v7.0.0 v6.3.0 labels Feb 20, 2018
This allows us to save a bit of code, but also adds more coverage as it tests serialization which was missing in some of the existing tests. Also it requires implementing equals/hashcode and we get the corresponding tests for them for free from the base test class.
@javanna javanna force-pushed the test/response_tests_xcontent_test_case branch from 73598ec to ad697dd Compare February 20, 2018 16:00
@cbuescher cbuescher self-assigned this Feb 20, 2018
@javanna
Copy link
Member Author

javanna commented Feb 20, 2018

retest this please

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.

LGTM


@Override
public boolean equals(Object o) {
if (super.equals(o)) {
Copy link
Member

Choose a reason for hiding this comment

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

I would like to double check this, I'm not sure if using super might violate the equals() contract.

Copy link
Member

@cbuescher cbuescher Feb 21, 2018

Choose a reason for hiding this comment

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

I think its all good, because AcknowledgedResponse is abstract. If it would be possible to create instances of it, I think there could have been cases where the symmetry property of equals() would be broken, but this cannot happen with non-instantiable superclasses.

Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

LGTM

@javanna javanna merged commit 8b4a298 into elastic:master Feb 21, 2018
javanna added a commit that referenced this pull request Feb 22, 2018
…8749)

This allows us to save a bit of code, but also adds more coverage as it tests serialization which was missing in some of the existing tests. Also it requires implementing equals/hashcode and we get the corresponding tests for them for free from the base test class.
martijnvg added a commit that referenced this pull request Feb 22, 2018
* es/master: (143 commits)
  Revert "Disable BWC tests for build issues"
  Remove AcknowledgedRestListener in favour of RestToXContentListener (#28724)
  Build: Consolidate archives and packages configuration (#28760)
  Skip some plugins service tests on Windows
  Migrate some *ResponseTests to AbstractStreamableXContentTestCase (#28749)
  Disable BWC tests for build issues
  Ensure that azure stream has socket privileges (#28751)
  [DOCS] Fixed broken link.
  Pass InputStream when creating XContent parser (#28754)
  [DOCS] Changed to use transient setting to reenabled allocation. Closes #27677
  Delay path expansion on Windows
  [TEST] replace randomAsciiAlphanumOfLengthBetween with randomAsciiLettersOfLengthBetween
  [Tests] Extract the testing logic for Google Cloud Storage (#28576)
  [Docs] Update links to java9 docs (#28750)
  version set in ingest pipeline (#27573)
  Revert "Add startup logging for standalone tests"
  Tests: don't wait for completion while trying to get completed task
  Add 5.6.9 snapshot version
  [Docs] Java high-level REST client : clean up (#28703)
  Updated distribution outputs in contributing docs
  ...
martijnvg added a commit that referenced this pull request Feb 22, 2018
* es/6.x: (140 commits)
  Remove AcknowledgedRestListener in favour of RestToXContentListener (#28724)
  Migrate some *ResponseTests to AbstractStreamableXContentTestCase (#28749)
  Revert "Disable BWC tests for build issues"
  Skip some plugins service tests on Windows
  Build: Consolidate archives and packages configuration (#28760)
  Ensure that azure stream has socket privileges (#28773)
  Disable BWC tests for build issues
  Pass InputStream when creating XContent parser (#28754)
  [DOCS] Fixed broken link.
  [DOCS] Changed to use transient setting to reenabled allocation. Closes #27677
  Delay path expansion on Windows
  [TEST] replace randomAsciiAlphanumOfLengthBetween with randomAsciiLettersOfLengthBetween
  REST high-level client: add support for Rollover Index API (#28698)
  [Tests] Extract the testing logic for Google Cloud Storage (#28576)
  Moved Grok helper code to a separate Gradle module and let ingest-common module depend on it.
  version set in ingest pipeline (#27573)
  [Docs] Update links to java9 docs (#28750)
  Revert "Add startup logging for standalone tests"
  Tests: don't wait for completion while trying to get completed task
  Add 5.6.9 snapshot version
  ...
sebasjm pushed a commit to sebasjm/elasticsearch that referenced this pull request Mar 10, 2018
…astic#28749)

This allows us to save a bit of code, but also adds more coverage as it tests serialization which was missing in some of the existing tests. Also it requires implementing equals/hashcode and we get the corresponding tests for them for free from the base test class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>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.

4 participants