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

testHlrcFromXContent() should respect assertToXContentEquivalence() #38232

Merged

Conversation

polyfractal
Copy link
Contributor

Tests can override assertToXContentEquivalence() in case their xcontent cannot be directly compared (e.g. due to insertion order in maps affecting the xcontent ordering). But the testHlrcFromXContent test hardcoded the equivalence test to true instead of consulting assertToXContentEquivalence()

@hub-cap tagged you for review because, while I think this is ok and good, I'm not entirely positive it wasn't intended to be hardcoded. Wanted another pair of eyeballs :)

Fixes #36034

Tests can override assertToXContentEquivalence() in case their xcontent
cannot be directly compared (e.g. due to insertion order in maps
affecting the xcontent ordering).  But the `testHlrcFromXContent` test
hardcoded the equivalence test to `true` instead of consulting
`assertToXContentEquivalence()`

Fixes elastic#36034
@polyfractal polyfractal added >test Issues or PRs that are addressing/adding tests :Core/Features/Java High Level REST Client labels Feb 1, 2019
@polyfractal polyfractal requested a review from hub-cap February 1, 2019 21:42
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@markharwood
Copy link
Contributor

Good spot, @polyfractal
I don't see why supportsUnknownFields, getShuffleFieldsExceptions, getRandomFieldsExcludeFilter and assertToXContentEquivalence should all be consulted but assertToXContentEquivalence isn't.
If the intended behaviour really is to hardcode true then it should be done by making assertToXContentEquivalence final and return true.

Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

nice catch!

@polyfractal polyfractal merged commit 54e684b into elastic:master Feb 5, 2019
polyfractal added a commit to polyfractal/elasticsearch that referenced this pull request Feb 5, 2019
Tests can override assertToXContentEquivalence() in case their xcontent
cannot be directly compared (e.g. due to insertion order in maps
affecting the xcontent ordering).  But the `testHlrcFromXContent` test
hardcoded the equivalence test to `true` instead of consulting
`assertToXContentEquivalence()`

Fixes elastic#36034
Backport of elastic#38232
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 5, 2019
* master: (23 commits)
  Lift retention lease expiration to index shard (elastic#38380)
  Make Ccr recovery file chunk size configurable (elastic#38370)
  Prevent CCR recovery from missing documents (elastic#38237)
  re-enables awaitsfixed datemath tests (elastic#38376)
  Types removal fix FullClusterRestartIT warnings (elastic#38445)
  Make sure to reject mappings with type _doc when include_type_name is false. (elastic#38270)
  Updates the grok patterns to be consistent with logstash (elastic#27181)
  Ignore type-removal warnings in XPackRestTestHelper (elastic#38431)
  testHlrcFromXContent() should respect assertToXContentEquivalence() (elastic#38232)
  add basic REST test for geohash_grid (elastic#37996)
  Remove DiscoveryPlugin#getDiscoveryTypes (elastic#38414)
  Fix the clock resolution to millis in GetWatchResponseTests (elastic#38405)
  Throw AssertionError when no master (elastic#38432)
  `if_seq_no` and `if_primary_term` parameters aren't wired correctly in REST Client's CRUD API (elastic#38411)
  Enable CronEvalToolTest.testEnsureDateIsShownInRootLocale (elastic#38394)
  Fix failures in BulkProcessorIT#testGlobalParametersAndBulkProcessor. (elastic#38129)
  SQL: Implement CURRENT_DATE (elastic#38175)
  Mute testReadRequestsReturnLatestMappingVersion (elastic#38438)
  [ML] Report index unavailable instead of waiting for lazy node (elastic#38423)
  Update Rollup Caps to allow unknown fields (elastic#38339)
  ...
polyfractal added a commit that referenced this pull request Feb 8, 2019
…38453)

Tests can override assertToXContentEquivalence() in case their xcontent
cannot be directly compared (e.g. due to insertion order in maps
affecting the xcontent ordering).  But the `testHlrcFromXContent` test
hardcoded the equivalence test to `true` instead of consulting
`assertToXContentEquivalence()`

Fixes #36034
Backport of #38232
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.7.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants