Skip to content

Fix failing test #20634

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Dec 20, 2022
Merged

Fix failing test #20634

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
05b61a1
Fix failing test
rodireich Dec 19, 2022
92667a9
Fix failing test
rodireich Dec 19, 2022
0fc9606
Fix failing test
rodireich Dec 19, 2022
163b8f3
Fix failing test
rodireich Dec 19, 2022
7e2629a
Fix failing test
rodireich Dec 19, 2022
902ff86
Fix failing test
rodireich Dec 20, 2022
2c80758
Fix failing test
rodireich Dec 20, 2022
eb2bb40
Fix failing test
rodireich Dec 20, 2022
736d2a1
Fix failing test
rodireich Dec 20, 2022
cc2dd2f
Merge branch 'master' into 19776-fix-failing-tests
rodireich Dec 20, 2022
e8007ca
comment
rodireich Dec 20, 2022
3847509
comment
rodireich Dec 20, 2022
a1839e6
Merge branch 'master' into 19776-fix-failing-tests
rodireich Dec 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ...a/io/airbyte/integrations/io/airbyte/integration_tests/sources/Db2SourceDatatypeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import io.airbyte.integrations.standardtest.source.TestDataHolder;
import io.airbyte.integrations.standardtest.source.TestDestinationEnv;
import io.airbyte.protocol.models.JsonSchemaType;
import java.math.BigDecimal;
import org.jooq.DSLContext;
import org.jooq.SQLDialect;
import org.testcontainers.containers.Db2Container;
Expand Down Expand Up @@ -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")))
Copy link
Contributor

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

Copy link
Contributor Author

@rodireich rodireich Dec 20, 2022 •
edited
Loading

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.

Copy link
Contributor

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

.build());
addDataTypeTestData(
TestDataHolder.builder()
Expand Down
8 changes: 5 additions & 3 deletions ...test-integration/java/io/airbyte/integrations/source/oracle/OracleSourceDatatypeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import io.airbyte.integrations.standardtest.source.TestDataHolder;
import io.airbyte.integrations.standardtest.source.TestDestinationEnv;
import io.airbyte.protocol.models.JsonSchemaType;
import java.math.BigDecimal;
import java.text.DateFormat;
import java.text.ParseException;
import java.text.SimpleDateFormat;
Expand Down Expand Up @@ -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")
Copy link
Contributor

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?

Copy link
Contributor Author

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 0s, so had to resort to direct string.
I'll add a comment.

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. thank you

.build());

addDataTypeTestData(
Expand All @@ -155,7 +157,7 @@ protected void initTests() {
.airbyteType(JsonSchemaType.NUMBER)
.fullSourceDataType("NUMBER(6,-2)")
.addInsertValues("123.89")
.addExpectedValues("100.0")
.addExpectedValues("100")
.build());

addDataTypeTestData(
Expand All @@ -164,7 +166,7 @@ protected void initTests() {
.airbyteType(JsonSchemaType.NUMBER)
.fullSourceDataType("FLOAT(5)")
.addInsertValues("1.34", "126.45")
.addExpectedValues("1.3", "130.0")
.addExpectedValues("1.3", "130")
.build());

addDataTypeTestData(
Expand Down