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

[EPIC] JDBC destination integration tests failing #15172

Closed
edgao opened this issue Aug 1, 2022 · 7 comments
Closed

[EPIC] JDBC destination integration tests failing #15172

edgao opened this issue Aug 1, 2022 · 7 comments
Assignees
Labels

Comments

@edgao
Copy link
Contributor

edgao commented Aug 1, 2022

These tests are failing due to a timestamp parsing error when verifying the output of normalization. See e.g. this test run, with errors like this:

PostgresDestinationAcceptanceTest > testDataTypeTestWithNormalization(String, String, TestCompatibility) > io.airbyte.integrations.destination.postgres.PostgresDestinationAcceptanceTest.testDataTypeTestWithNormalization(String, String, TestCompatibility)[1] FAILED
    java.time.format.DateTimeParseException: Text '2022-11-22 01:23:45.0' could not be parsed at index 10
        at java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2052)
        at java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1954)
        at java.base/java.time.LocalDate.parse(LocalDate.java:430)
        at io.airbyte.integrations.destination.postgres.PostgresTestDataComparator.parseLocalDate(PostgresTestDataComparator.java:40)
        at io.airbyte.integrations.destination.postgres.PostgresTestDataComparator.compareDateTimeValues(PostgresTestDataComparator.java:48)
        at io.airbyte.integrations.standardtest.destination.comparator.AdvancedTestDataComparator.compareJsonNodes(AdvancedTestDataComparator.java:89)
        at io.airbyte.integrations.standardtest.destination.comparator.AdvancedTestDataComparator.assertSameValue(AdvancedTestDataComparator.java:76)

This started happening after #13591. Previously, we were receiving timestamps like "2022-11-22T01:23:45" (note the T between the date and time, and the lack of decimals).

What needs to happen on this ticket?

  • Run manual syncs to verify that destination-postgres and destination-mysql (+normalization) are correctly persisting timestamp/timestamptz (this should be fine, but want to be extra careful)
  • Assuming they work correctly: Update the *TestDataComparator to be able to parse the new timestamp format
@andriikorotkov
Copy link
Contributor

@edgao this issue affects not only Postgres destination but also all JDBC destinations. You can test this by running acceptance tests, for example, on the mysql destination. We will get the same error. Maybe this information will help you.

@edgao edgao changed the title destination postgres tests broken JDBC destination integration tests failing Aug 2, 2022
@edgao
Copy link
Contributor Author

edgao commented Aug 2, 2022

finally confirmed that #13591 is the cause (was able to check out the previous commit and run tests successfully).

Need to determine if this is a problem in normalization or just a problem with how the tests try to parse the timestamps.

@edgao
Copy link
Contributor Author

edgao commented Aug 2, 2022

updated description with more information about what's happening.

@grishick I think we should prioritize this somewhat highly, since it's blocking /publish on all JDBC destinations, but at least it shouldn't be breaking anything in production.

@rodireich rodireich pinned this issue Aug 3, 2022
@etsybaev etsybaev self-assigned this Aug 3, 2022
@andriikorotkov andriikorotkov moved this to Blocked in GL Roadmap Aug 3, 2022
@etsybaev etsybaev self-assigned this Aug 3, 2022
@etsybaev etsybaev added the Epic label Aug 3, 2022
@etsybaev etsybaev changed the title JDBC destination integration tests failing [EPIC] JDBC destination integration tests failing Aug 3, 2022
@grishick
Copy link
Contributor

grishick commented Aug 3, 2022

Destination-postgres: fix java-based normalization tests for datetime

agreed. Do you know if the fix will be in normalization or in shared jdbc destination code or in each jdbc destination separately?

@edgao
Copy link
Contributor Author

edgao commented Aug 3, 2022

I think it'll be in the shared jdbc destination test code, and potentially each jdbc destination's test implementation. Either the Basic or Advanced TestDataComparator might be the right place to start.

The * TestDataComparator classes that I mentioned in the description seem to exist in several (but not all) JDBC destinations, and they might need additional work to fix.

@edgao
Copy link
Contributor Author

edgao commented Aug 4, 2022

looks like destination-postgres was fixed here #15289. I would expect other destinations to look pretty similar.

@etsybaev
Copy link
Contributor

etsybaev commented Aug 7, 2022

For now, I see 4 jdbc connectors affected by this Python change. Created sub-task for them. For now I'm adding fixes individually to unblock others and deliver fixes ASAP. When it comes to the last one I will check if it's possible to move out something to the parent module because fixes seem to be similar but not the same for all of connectos
#15247
#15246
#15245
#15236

@etsybaev etsybaev closed this as completed Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Blocked
Development

No branches or pull requests

4 participants