Fix bug "Assertion framework(Yaml Rest test): numeric comparison fails when comparing Integer vs Long (or Float vs Double)" #19376
Conversation
…ls when comparing Integer vs Long (or Float vs Double) Signed-off-by: Lamine Idjeraoui <[email protected]>
Signed-off-by: Lamine Idjeraoui <[email protected]>
github-actions
bot
added
bug
|
❌ Gradle check result for e258963: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
Hi @sandeshkr419 |
| return Double.parseDouble(actualValue.toString()); | ||
| } else if (expectedValue instanceof Integer) { | ||
| return Integer.parseInt(actualValue.toString()); | ||
| try { |
There was a problem hiding this comment.
Trying to understand the intent of this try block in int case, is this to handle big integer cases? And the catch block potentially providing the next best match for parsing actual value?
| */ | ||
| private static Tuple<Object, Object> normalizePair(Object a, Object b) { | ||
| if (a instanceof Number && b instanceof Number) { | ||
| boolean floaty = (a instanceof Float) || (a instanceof Double) || (b instanceof Float) || (b instanceof Double); |
| return Tuple.tuple(a, b); | ||
| } | ||
|
|
||
| private static Double asDouble(Object n) { |
There was a problem hiding this comment.
Can we force this method to take in Number argument? Can directly type-cast when checking instanceof. Can save a check and tighten up the usage as well.
private static Tuple<Object, Object> normalizePair(Object a, Object b) {
if (a instanceof Number aNum && b instanceof Number bNum) {
| return Double.parseDouble(n.toString()); | ||
| } | ||
|
|
||
| private static Long asLong(Object n) { |
There was a problem hiding this comment.
Can we force this method to take in Number argument?
|
|
||
| private static final Logger logger = LogManager.getLogger(OrderingAssertion.class); | ||
|
|
||
| protected enum Relation { |
There was a problem hiding this comment.
Does OrderingAssertion needs to be more generic by handling EXACT case as well, If there are any?
| ); | ||
| } | ||
| return new LessThanOrEqualToAssertion(location, stringObjectTuple.v1(), stringObjectTuple.v2()); | ||
| return parseCommon(parser, "lte", LessThanOrEqualToAssertion::new); |
There was a problem hiding this comment.
Didn't go into super details, but is it not feasible to pass Relation.LTE enum directly instead of the lte string?
| /** | ||
| * Common parser for {gt|gte|lt|lte}: { field: expectedValue } | ||
| */ | ||
| protected static <T extends OrderingAssertion> T parseCommon(XContentParser parser, String sectionName, OrderingAssertionFactory<T> factory) |
There was a problem hiding this comment.
nit: more descriptive name for the method? Probably parseOrderingAssertion or anything else?
sandeshkr419
added
skip-changelog
v3.4.0
2 participants
Resolves #19323
The numerical comparison classes (gt, gte, lt and lte) share more than 90% of the code. Refactored the 4 classes by extracted the common code into a super class.
Updated the convertActualValue method to normalize mixed numeric types to a common upper type.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.