-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fix failing test #20634
Fix failing test #20634
Conversation
/test connector=connectors/source-oracle |
/test connector=connectors/source-db2
Build FailedTest summary info:
|
/test connector=connectors/source-oracle
Build FailedTest summary info:
|
/test connector=connectors/source-oracle
Build FailedTest summary info:
|
/test connector=connectors/source-db2 |
/test connector=connectors/source-db2
Build PassedTest summary info:
|
/test connector=connectors/source-oracle
Build FailedTest summary info:
|
/test connector=connectors/source-oracle
|
/test connector=connectors/source-oracle
Build FailedTest summary info:
|
/test connector=connectors/source-oracle
Build FailedTest summary info:
|
/test connector=connectors/source-oracle
Build PassedTest summary info:
|
@@ -146,7 +147,8 @@ protected void initTests() { | |||
.sourceType("NUMBER") | |||
.airbyteType(JsonSchemaType.NUMBER) | |||
.addInsertValues("null", "1", "123.45", "power(10, -130)", "9.99999999999999999999 * power(10, 125)") | |||
.addExpectedValues(null, "1", "123.45", String.valueOf(Math.pow(10, -130)), String.valueOf(9.99999999999999999999 * Math.pow(10, 125))) | |||
.addExpectedValues(null, "1", "123.45", String.valueOf(Math.pow(10, -130)), | |||
"999999999999999999999000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not possible to have the last expected value be
String.valueOf(9.99999999999999999999 * Math.pow(10, 125)
This would be difficult to understand, but if the usage of Math.pow(10,125)
is not an option can we add a comment to this test case explaining how come this string is the way it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The String.valueOf
we used to have was printed as "1.0E126" - the scientific notation.
I couldn't find a way to compel BigDecimal to accept a value using pow
that is not printed as a 1
with 126 0
s, so had to resort to direct string.
I'll add a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might add in the comment the reason how come it's a plain string representation, like something along the lines of
"because normalization expects values in integer strings whereas `Math.pow(10, 125)` returns a scientific notation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. thank you
...rc/test-integration/java/io/airbyte/integrations/source/oracle/OracleSourceDatatypeTest.java
Outdated
Show resolved
Hide resolved
@@ -126,7 +127,7 @@ protected void initTests() { | |||
.airbyteType(JsonSchemaType.NUMBER) | |||
.fullSourceDataType("DECIMAL(31, 0)") | |||
.addInsertValues("null", "1", "DECIMAL((-1 + 10E+29), 31, 0)", "DECIMAL((1 - 10E+29), 31, 0)") | |||
.addExpectedValues(null, "1", "1.0E30", "-1.0E30") | |||
.addExpectedValues(null, "1", "%.0f".formatted(Double.valueOf("1.0E30")), "%.0f".formatted(Double.valueOf("-1.0E30"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding this correctly, this will be showing integer values only, right? I haven't seen the usage of %.0f
before, normally it would have one decimal precision value.
Also this seems to work but curious if this should be
Float.valueOf("1.0E30")
since the string formatting is indicating a Float f
vs Double d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The weeds of floating point string format.
How I missed thee… :-)
f
signifies a floating point number - a d
modifier is a decimal integer I think, but it won't accept a double or float value.
the %.0f
formats a floating point number to print with 0 decimal digits (0 precision) - which is the expected string.
And a Float.valueOf("1.0E30")
is theoretically ok, but depending on actual hardware and implementation won't print as a clean 1
with zeroes but some weird floating point "1000000015047466200000000000000" and so on- it's the whole single/double precision implementation of respective float/double types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation, nothing else to add and glad for the clarification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, minor nit and comments to make sure business context is clear
/approve-and-merge reason="Low risk: Fixing test only code that's broken today to unblock the team" |
What
Fix failing tests in source-oracle and source-db2 due to previous change in #19776