Skip to content

Conversation

@martijnvg
Copy link
Member

Relates to #24939

Copy link
Member

Choose a reason for hiding this comment

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

I'd go with assertThat(totalHits, greaterThanOrEqualTo(1)) because it is going to produce a nice error message.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, on the other hand I try to use junit's Assert.* methods where possible. I'm not a big fan of using both test libraries in the same test case. But in this case it does improve readability.

Copy link
Member

Choose a reason for hiding this comment

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

I go the other way: I use junit's assert when it produces goot error messages and hamcrest otherwise. Whatever works is cool.

Copy link
Member

Choose a reason for hiding this comment

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

Map<?, ?> bestHit = (Map<?, ?>) ... will probably work and won't require the SuppressWarnings.

Copy link
Member

Choose a reason for hiding this comment

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

Probably....

Copy link
Member

Choose a reason for hiding this comment

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

I do this a bit in other places. Do you want to convert? I didn't feel the need to make this but it is shorter so I'm fine with it.

Copy link
Member

Choose a reason for hiding this comment

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

I feel like assertNewReplicasWork should be in its own test. It doesn't have much to do with searching, I don't think.

Copy link
Member

Choose a reason for hiding this comment

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

I just like the idea of keeping the complexity around searching in the search test. That way the test for new replicas doesn't need fancy documents. It doesn't have fancy documents so we can be sure fancy documents have nothing to do with the test.

@martijnvg martijnvg force-pushed the port_more_bwc_tests_to_full_cluster_restart_test branch from 0e1372f to 21ab817 Compare June 14, 2017 08:43
@martijnvg
Copy link
Member Author

@nik9000 Thanks! I've update the PR.

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. I did just merge #25204 which causes some conflicts. Sorry!

@martijnvg martijnvg force-pushed the port_more_bwc_tests_to_full_cluster_restart_test branch from 21ab817 to cf2f802 Compare June 15, 2017 11:33
@martijnvg martijnvg force-pushed the port_more_bwc_tests_to_full_cluster_restart_test branch from cf2f802 to 7b75b37 Compare June 15, 2017 11:33
@martijnvg martijnvg merged commit fe02829 into elastic:master Jun 15, 2017
jasontedor added a commit to glefloch/elasticsearch that referenced this pull request Jun 15, 2017
* master:
  [Test] restore BWC for parent-join now that the new mapping format is in 5.x
  Add a section named "relations" in the ParentJoinFieldMapper (elastic#25248)
  test: Ported more OldIndexBackwardsCompatibilityIT tests to full cluster restart qa tests. (elastic#25173)
  fix: Sort Processor does not have proper behavior with targetField (elastic#25237)
  Allow reader wrappers to have different live docs but the same cache key.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jun 16, 2017
…y-context

* 'master' of github.com:elastic/elasticsearch: (21 commits)
  [DOCS] Clarify expected availability of HDFS for the HDFS Repository (elastic#25220)
  Remove some redundant 140 character checkstyle suppressions
  [Docs] more fix for the parent-join docs
  [Docs] Fix cross reference for parent-join field
  More advices around search speed and disk usage. (elastic#25252)
  Add documentation for the new parent-join field (elastic#25227)
  [analysis-icu] Allow setting unicodeSetFilter (elastic#20814)
  Introduce translog size and age based retention policies (elastic#25147)
  Add needs methods for specific variables to Painless script context factories. (elastic#25267)
  Improves snapshot logging and snapshoth deletion error handling (elastic#25264)
  Add unit test for PathHierarchyTokenizerFactory (elastic#24984)
  Deprecate tribe service
  Moved more token filters to analysis-common module.
  [Test] Make sure that SearchAfterSortedDocQueryTests uses a single threaded searcher
  [DOCS] Defined es-test-dir and plugins-examples-dir in index.asciidoc.  (elastic#25232)
  Test fix - removed superfluous assertion (elastic#25247)
  [Test] restore BWC for parent-join now that the new mapping format is in 5.x
  Add a section named "relations" in the ParentJoinFieldMapper (elastic#25248)
  test: Ported more OldIndexBackwardsCompatibilityIT tests to full cluster restart qa tests. (elastic#25173)
  fix: Sort Processor does not have proper behavior with targetField (elastic#25237)
  ...
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.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants