[source-postgres]Non CDC CAT test for postgres #41650

xiaohansong · 2024-07-11T23:01:30Z

What

acceptance test for non cdc postgres config.

How

hook.py as a setup script will generate a schema name. Acceptance tests are running async thus it could cause some confliction if we setup a new schema each time for a new test. Instead we have more flexibility in hook.py and we may be able to generate multiple schema if needed.
All tests in one CAT test run will share the same schema.
teardown logic will only clean up schema from previous days.
Fix a bug on incremental test where we made incorrect assumption on DB source cursor state

Review guide

User Impact

Can this PR be safely reverted and rolled back?

YES 💚
NO ❌

vercel · 2024-07-11T23:01:34Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Jul 24, 2024 7:08pm

CLAassistant · 2024-07-12T17:07:25Z

All committers have signed the CLA.

clnoll

I didn't review the database-specific code too closely, but overall it's looking good! Just a few comments/questions.

airbyte-integrations/connectors/source-postgres/integration_tests/Dockerfile

airbyte-integrations/connectors/source-postgres/integration_tests/abnormal_state_copy.json

airbyte-integrations/connectors/source-postgres/integration_tests/seed/hook.py

test

...grations/bases/connector-acceptance-test/connector_acceptance_test/tests/test_incremental.py

airbyte-integrations/connectors/source-postgres/integration_tests/abnormal_state.json

theyueli · 2024-07-16T09:18:59Z

airbyte-integrations/connectors/source-postgres/integration_tests/seed/hook.py

+    today = datetime.datetime.now()
+    yesterday = today - timedelta(days=1)
+    formatted_yesterday = yesterday.strftime('%y%m%d')
+    delete_schemas_with_prefix(connection, formatted_yesterday)


typically, teardown is for deleting stuff we create in this test. Is it possible to just remove the schema we just created?

it's tricky, because incremental test have multiple tests running at the same time. Deleting the schema could cause other tests to fail.

another question, you mentioned we work on the same schema, if that's the case, when we run concurrent testing, would this deletion affects other tests?

theyueli · 2024-07-16T09:25:36Z

airbyte-integrations/connectors/source-postgres/integration_tests/seed/hook.py

+    with open("./generated_schema.txt", "r") as f:
+        return f.read()
+
+def remove_all_write_files():


do we need to perform this step? I thought when container gets destroyed after test is done, these files will be gone as they are part of the container.

We ask the setup container to port files back to directory because config needs to be dynamically generated (to incorporate schema). thus this is needed. However this is not working right not and @clnoll is working on that!

theyueli · 2024-07-16T09:27:57Z

airbyte-integrations/connectors/source-postgres/integration_tests/seed/hook.py

+    elif command == "teardown":
+        teardown()
+    elif command == "prepare":
+        prepare()


to simplify, maybe we shall just make prepare() as part of setup()? setup to me means all the preparation work...

prepare is supposed to run once before we start CAT test, while setup is going to run before each test.

Reason being we realized it's easier to work on a single schema among the entire test suite, to avoid that async incremental test conflictions.

theyueli · 2024-07-16T09:31:52Z

airbyte-integrations/connectors/source-postgres/integration_tests/seed/hook.py

+
+    if connection:
+        # Create the schema
+        create_schema(connection, schema_name)


ideally, we shall check the return status of each step here, and if one step failed, we shall log, close the connection without the rest of the steps.

I think in each step it will close connection whether or not it would success. And since we don't have try catch we would always throw exceptions - I think exceptions are easier to debug

airbyte-integrations/connectors/source-postgres/integration_tests/seed/hook.py

airbyte-integrations/connectors/source-postgres/integration_tests/seed/init.sql

...ns/connectors/source-postgres/integration_tests/incremental_configured_catalog_template.json

theyueli · 2024-07-18T23:46:59Z

looks ready mostly to me, @xiaohansong please make sure we run a couple of tests in parallel to make sure this will not have any issue.

theyueli

Looks good to me if the concurrent tests are able to pass.

…42352)

…20] (#42347)

…onfig objects (#42015)

implementing refreshes for destination-postgres we're bumping the CDK version to the latest, and modifying a whole lot of jsonl files for tests (in both regular and strict-encrypt)

theyueli

Looks good!

clnoll

Nice @xiaohansong! Just a few more comments for you.

airbyte-integrations/connectors/source-postgres/integration_tests/seed/hook.py

clnoll · 2024-07-23T19:34:17Z

airbyte-integrations/connectors/source-postgres/integration_tests/seed/hook.py

+        return connection
+    except Exception as error:
+        print(f"Error connecting to the database: {error}")
+        return None


Is there any reason not to just let the exception get raised here, or exit(1)? Otherwise the None will cause a failure downstream anyway.

Removing the None return value will also help simplify the downstream code since we won't have to check that the return value exists before use.

Also (especially if you're going to sometimes return an optional value) I'd really recommend using type annotations. It's not too burdensome to add them and they'll offer protection from a large category of unhandled exceptions.

You can find examples of how to use them throughout airbyte_cdk.

clnoll · 2024-07-23T19:43:37Z

airbyte-integrations/connectors/source-postgres/integration_tests/seed/hook.py

+    delete_schemas_with_prefix(connection, formatted_yesterday)
+
+if __name__ == "__main__":
+    command = sys.argv[1]


Nit: we should probably raise an exception if the command isn't "setup", "teardown", or "prepare", so the script doesn't exit silently if there's a regression.

…sts/seed/hook.py Co-authored-by: Catherine Noll <clnoll@users.noreply.github.com>

octavia-squidington-iii added area/connectors Connector related issues connectors/source/postgres labels Jul 11, 2024

octavia-squidington-iii added area/documentation Improvements or additions to documentation CDK Connector Development Kit and removed area/documentation Improvements or additions to documentation CDK Connector Development Kit labels Jul 12, 2024

vercel bot deployed to Preview July 12, 2024 17:12 View deployment

xiaohansong requested review from clnoll and theyueli July 12, 2024 21:31

clnoll reviewed Jul 15, 2024

View reviewed changes

theyueli reviewed Jul 16, 2024

View reviewed changes

vercel bot deployed to Preview July 18, 2024 20:46 View deployment

xiaohansong marked this pull request as ready for review July 18, 2024 20:59

xiaohansong requested a review from a team as a code owner July 18, 2024 20:59

theyueli reviewed Jul 18, 2024

View reviewed changes

...ns/connectors/source-postgres/integration_tests/incremental_configured_catalog_template.json Outdated Show resolved Hide resolved

theyueli approved these changes Jul 21, 2024

View reviewed changes

alafanechere added 11 commits July 23, 2024 10:21

🐙 source-oura: run up-to-date pipeline [2024-07-20] (#42356)

d338a6a

🐙 destination-xata: run up-to-date pipeline [2024-07-20] (#42355)

fbf079d

🐙 destination-timeplus: run up-to-date pipeline [2024-07-20] (#42354)

5d596f6

🐙 source-strava: run up-to-date pipeline [2024-07-20] (#42353)

976aea3

🐙 source-open-exchange-rates: run up-to-date pipeline [2024-07-20] (#…

04c2786

…42352)

🐙 source-coda: run up-to-date pipeline [2024-07-20] (#42351)

d799c6f

🐙 source-my-hours: run up-to-date pipeline [2024-07-20] (#42350)

dd02b45

🐙 source-chartmogul: run up-to-date pipeline [2024-07-20] (#42349)

7f9abf7

🐙 source-exchange-rates: run up-to-date pipeline [2024-07-20] (#42348)

334dbcf

🐙 source-google-analytics-data-api: run up-to-date pipeline [2024-07-…

7fdc342

…20] (#42347)

🐙 source-instagram: run up-to-date pipeline [2024-07-20] (#42346)

e31d597

stephane-airbyte and others added 10 commits July 23, 2024 10:21

cdk-java: wait until after table migration before creating the WriteC…

633bc82

…onfig objects (#42015)

cdk-java: bump postgresql version (#42017)

3d500a2

cdk-java: fix RecordDiffer (#42117)

461253d

destination-postgres: implement refreshes (#41954)

df624fb

implementing refreshes for destination-postgres we're bumping the CDK version to the latest, and modifying a whole lot of jsonl files for tests (in both regular and strict-encrypt)

airbyte-ci: skip regression test approval requirement on forks (#42410)

d5739a5

source-singlestore: set useLocalCdk=false (#42033)

7e703f3

live tests: docs update (#42408)

8f9ab0f

CATs: only export /integration_tests

4194c16

update

13831ee

Merge remote-tracking branch 'origin/master' into xiaohan/test

14afca1

xiaohansong requested a review from a team as a code owner July 23, 2024 17:21

octavia-squidington-iii added connectors/destination/postgres connectors/destination/postgres-strict-encrypt connectors/source/zendesk-support labels Jul 23, 2024

vercel bot deployed to Preview July 23, 2024 17:26 View deployment

xiaohansong added 5 commits July 23, 2024 10:49

reset and format

957c395

format

aaf6eca

fix

2d0906f

check core

cb7b75e

typo

3f1c313

theyueli approved these changes Jul 23, 2024

View reviewed changes

clnoll reviewed Jul 23, 2024

View reviewed changes

xiaohansong and others added 4 commits July 23, 2024 14:57

Update airbyte-integrations/connectors/source-postgres/integration_te…

e2402ed

…sts/seed/hook.py Co-authored-by: Catherine Noll <clnoll@users.noreply.github.com>

comment PR and release ci

a96a68a

add test

fbfdcfd

Merge branch 'master' into xiaohan/test

8dcefef

vercel bot deployed to Preview July 24, 2024 16:48 View deployment

fix test

552a027

xiaohansong merged commit 8bf87e8 into master Jul 24, 2024
36 checks passed

xiaohansong deleted the xiaohan/test branch July 24, 2024 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[source-postgres]Non CDC CAT test for postgres #41650

[source-postgres]Non CDC CAT test for postgres #41650

xiaohansong commented Jul 11, 2024 •

edited

Loading

vercel bot commented Jul 11, 2024 •

edited

Loading

CLAassistant commented Jul 12, 2024 •

edited

Loading

clnoll left a comment

theyueli Jul 16, 2024

xiaohansong Jul 17, 2024

theyueli Jul 18, 2024

theyueli Jul 16, 2024

xiaohansong Jul 17, 2024

theyueli Jul 16, 2024

xiaohansong Jul 17, 2024

theyueli Jul 16, 2024

xiaohansong Jul 18, 2024

theyueli commented Jul 18, 2024

theyueli left a comment

theyueli left a comment

clnoll left a comment

clnoll Jul 23, 2024

clnoll Jul 23, 2024

clnoll Jul 23, 2024

[source-postgres]Non CDC CAT test for postgres #41650

[source-postgres]Non CDC CAT test for postgres #41650

Conversation

xiaohansong commented Jul 11, 2024 • edited Loading

What

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

vercel bot commented Jul 11, 2024 • edited Loading

CLAassistant commented Jul 12, 2024 • edited Loading

clnoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theyueli commented Jul 18, 2024

theyueli left a comment

Choose a reason for hiding this comment

theyueli left a comment

Choose a reason for hiding this comment

clnoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaohansong commented Jul 11, 2024 •

edited

Loading

vercel bot commented Jul 11, 2024 •

edited

Loading

CLAassistant commented Jul 12, 2024 •

edited

Loading