Enable full SAT for the Redshift source #19915

DoNotPanicUA · 2022-11-30T12:05:13Z

What

Enable SAT for the Redshift source

Configured test instances

sat_test_dataset

Found issues

SAT related issues:

in case of default value for the configured_catalog_path, the SAT doesn't take the file by default, but sync all available streams from the source.

…redshift

DoNotPanicUA · 2022-11-30T16:08:08Z

✔️ SAT passed. The master branch contains issues in the Status report.

/test connector=connectors/source-redshift

🕑 connectors/source-redshift https://github.com/airbytehq/airbyte/actions/runs/3585021128
❌ connectors/source-redshift https://github.com/airbytehq/airbyte/actions/runs/3585021128
🐛

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_incremental.py:26: `future_state` not specified, skipping.
============ 29 passed, 1 skipped, 31 warnings in 396.11s (0:06:36) ============

evantahler

👍

Similar question to BQ - should we document the test instance's details in some way

evantahler · 2022-11-30T21:04:39Z

airbyte-integrations/connectors/source-redshift/acceptance-test-config.yml

@@ -4,4 +4,26 @@ connector_image: airbyte/source-redshift:dev
 tests:
  spec:
    - spec_path: "src/test-integration/resources/expected_spec.json"
+      timeout_seconds: "1200"


lol, redshift.

DoNotPanicUA · 2022-12-02T16:49:38Z

/test connector=connectors/source-redshift

🕑 connectors/source-redshift https://github.com/airbytehq/airbyte/actions/runs/3603527916
✅ connectors/source-redshift https://github.com/airbytehq/airbyte/actions/runs/3603527916
Python tests coverage:

	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          12      4    67%   16-19
	 source_acceptance_test/config.py                       140      5    96%   87, 93, 238, 242-243
	 source_acceptance_test/conftest.py                     199     93    53%   35, 41-43, 48, 53, 59, 65, 71, 77-79, 98, 103-105, 111-113, 119-120, 125-126, 131, 137, 146-155, 161-166, 181, 205, 236, 242, 250-255, 263-268, 276-289, 294-300, 307-318, 325-341
	 source_acceptance_test/plugin.py                        69     25    64%   22-23, 31, 36, 120-140, 144-148
	 source_acceptance_test/tests/test_core.py              398    111    72%   53, 58, 87-95, 100-107, 111-112, 116-117, 299, 337-354, 363-371, 375-380, 386, 419-424, 462-469, 512-514, 517, 582-590, 602-605, 610, 666-667, 673, 676, 712-722, 735-760
	 source_acceptance_test/tests/test_incremental.py       158     14    91%   52-59, 64-77, 240
	 source_acceptance_test/utils/asserts.py                 39      2    95%   62-63
	 source_acceptance_test/utils/common.py                  94     10    89%   16-17, 32-38, 72, 75
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       112     50    55%   23-26, 32, 36, 39-68, 71-73, 76-78, 81-83, 86-88, 91-93, 96-114, 148-150
	 source_acceptance_test/utils/json_schema_helper.py     107     13    88%   30-31, 38, 41, 65-68, 96, 120, 192-194
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1569    350    78%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_incremental.py:26: `future_state` not specified, skipping.
============ 29 passed, 1 skipped, 31 warnings in 200.30s (0:03:20) ============

DoNotPanicUA · 2022-12-06T15:39:03Z

+1

Similar question to BQ - should we document the test instance's details in some way

the doc.

alafanechere · 2022-12-06T16:31:37Z

+1
Similar question to BQ - should we document the test instance's details in some way

the doc.

I think we shall create connector-specific docs for better visibility for our community. I would suggest adding a README under integration-tests + a source specific script

airbyte-integrations/connectors/source-redshift/acceptance-test-config.yml

DoNotPanicUA · 2022-12-07T14:41:37Z

/test connector=connectors/source-redshift

🕑 connectors/source-redshift https://github.com/airbytehq/airbyte/actions/runs/3639930982
❌ connectors/source-redshift https://github.com/airbytehq/airbyte/actions/runs/3639930982
🐛

alafanechere · 2022-12-07T15:09:20Z

airbyte-integrations/connectors/source-redshift/integration_tests/README.md

+You can find the SQL scripts in this folder if you need to create or fix the SAT dataset.
+For more instructions and information about valid scripts, please check this [doc](https://docs.google.com/document/d/1k5TvxaNhKdr44aJIHWWtLk14Tzd2gbNX-J8YNoTj8u0/edit#heading=h.ls9oiedt9wyy).


I don't think we should link a Google Doc here, it's not or usual approach to documentation and can break accessibility.

Moreover I would suggest you add the following to the README:

Share the requirements in terms of Redshift setup to run the tests on it.

Explain the role of each SQL script.

Explain technically how to seed the DB.

I think these instructions will differ for each source, so I'm not in favor of a central document. If there are common instructions that can be useful for all database sources, feel free to edit https://github.com/airbytehq/airbyte/blob/master/docs/connector-development/testing-connectors/source-acceptance-tests-reference.md#L1

That is the point, all instructions are the same. The difference is only in the credentials required for the connection and the SQL syntax. It's common for any SQL source.
@evantahler created the doc and said to put all related instructions there.

P.S. If we have a new demand to improve or rework something, it's better to create a new issue and discuss it at the planning meeting with the whole team. No need to hold the integration tests for the source as a hostage of this discussion.

If I'm being asked to be a tie-breaker, in the spirt of "done is better than perfect", I think the google doc is OK for now. We can move the contents into the public repo once we are done with all the Java sources, and we can see how similar/different the instructions will be. Also, the doc currently links to secrets, which is helpful, and shouldn't be in this repo.

alafanechere · 2022-12-07T21:05:02Z

/test connector=connectors/source-redshift

🕑 connectors/source-redshift https://github.com/airbytehq/airbyte/actions/runs/3642819153
❌ connectors/source-redshift https://github.com/airbytehq/airbyte/actions/runs/3642819153
🐛 https://gradle.com/s/ekzst7wglpa7y

Build Failed

Test summary info:

Could not find result summary

alafanechere

I updated your branch to get the test working again. Feel free to merge once they pass.

DoNotPanicUA · 2022-12-08T11:56:53Z

/test connector=connectors/source-redshift

🕑 connectors/source-redshift https://github.com/airbytehq/airbyte/actions/runs/3648031147
✅ connectors/source-redshift https://github.com/airbytehq/airbyte/actions/runs/3648031147
Python tests coverage:

	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          12      4    67%   16-19
	 source_acceptance_test/config.py                       140      5    96%   87, 93, 238, 242-243
	 source_acceptance_test/conftest.py                     208     92    56%   36, 42-44, 49, 54, 77, 83, 89-91, 110, 115-117, 123-125, 131-132, 137-138, 143, 149, 158-167, 173-178, 193, 217, 248, 254, 262-267, 275-280, 288-301, 306-312, 319-330, 337-353
	 source_acceptance_test/plugin.py                        69     25    64%   22-23, 31, 36, 120-140, 144-148
	 source_acceptance_test/tests/test_core.py              398    111    72%   53, 58, 87-95, 100-107, 111-112, 116-117, 299, 337-354, 363-371, 375-380, 386, 419-424, 462-469, 512-514, 517, 582-590, 602-605, 610, 666-667, 673, 676, 712-722, 735-760
	 source_acceptance_test/tests/test_incremental.py       158     14    91%   52-59, 64-77, 240
	 source_acceptance_test/utils/asserts.py                 39      2    95%   62-63
	 source_acceptance_test/utils/common.py                  94     10    89%   16-17, 32-38, 72, 75
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       133     33    75%   24-27, 46-47, 50-54, 57-58, 73-75, 78-80, 83-85, 88-90, 93-95, 124-125, 159-161, 208
	 source_acceptance_test/utils/json_schema_helper.py     107     13    88%   30-31, 38, 41, 65-68, 96, 120, 192-194
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1599    332    79%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/tests/test_incremental.py:26: `future_state` not specified, skipping.
================== 29 passed, 1 skipped in 384.72s (0:06:24) ===================

DoNotPanicUA added 2 commits November 30, 2022 13:59

enable SAT for redshift

8f3be7d

Merge remote-tracking branch 'origin/master' into aleonets/19895-SAT-…

16d717f

…redshift

DoNotPanicUA changed the title ~~Aleonets/19895 sat redshift~~ Enable full SAT for the Redshift source Nov 30, 2022

octavia-squidington-iv added area/connectors Connector related issues connectors/source/redshift labels Nov 30, 2022

DoNotPanicUA linked an issue Nov 30, 2022 that may be closed by this pull request

Enable SAT for hosted sources - Redshift #19895

Closed

This comment was marked as outdated.

Sign in to view

add bigger timeouts + specify catalogs (default works wrong)

0c55000

DoNotPanicUA marked this pull request as ready for review November 30, 2022 17:31

DoNotPanicUA requested a review from a team as a code owner November 30, 2022 17:31

Merge branch 'master' into aleonets/19895-SAT-redshift

1d5527e

DoNotPanicUA requested review from evantahler and a team November 30, 2022 17:31

evantahler reviewed Nov 30, 2022

View reviewed changes

DoNotPanicUA and others added 2 commits December 2, 2022 15:05

new line

fe9266f

Merge branch 'master' into aleonets/19895-SAT-redshift

f01c8cf

DoNotPanicUA requested a review from evantahler December 6, 2022 15:39

alafanechere requested changes Dec 6, 2022

View reviewed changes

airbyte-integrations/connectors/source-redshift/acceptance-test-config.yml Outdated Show resolved Hide resolved

DoNotPanicUA added 2 commits December 7, 2022 16:16

readme

c8a65a1

new format

b6d3ec1

DoNotPanicUA requested a review from alafanechere December 7, 2022 14:42

alafanechere requested changes Dec 7, 2022

View reviewed changes

Merge branch 'master' into aleonets/19895-SAT-redshift

8531d86

alafanechere approved these changes Dec 7, 2022

View reviewed changes

Merge branch 'master' into aleonets/19895-SAT-redshift

19ce10b

DoNotPanicUA merged commit c2e4c3e into master Dec 8, 2022

DoNotPanicUA deleted the aleonets/19895-SAT-redshift branch December 8, 2022 18:24

github-actions bot mentioned this pull request Dec 8, 2022

Bump helm chart version reference to 0.43.0 #20268

Closed

octavia-squidington-iii mentioned this pull request Dec 9, 2022

Bump Airbyte version from 0.40.24 to 0.40.25 #20279

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable full SAT for the Redshift source #19915

Enable full SAT for the Redshift source #19915

DoNotPanicUA commented Nov 30, 2022 •

edited

Loading

This comment was marked as outdated.

DoNotPanicUA commented Nov 30, 2022 •

edited

Loading

evantahler left a comment •

edited

Loading

evantahler Nov 30, 2022

DoNotPanicUA commented Dec 2, 2022 •

edited by github-actions bot

Loading

DoNotPanicUA commented Dec 6, 2022

alafanechere commented Dec 6, 2022

DoNotPanicUA commented Dec 7, 2022 •

edited by github-actions bot

Loading

alafanechere Dec 7, 2022 •

edited

Loading

DoNotPanicUA Dec 7, 2022

evantahler Dec 7, 2022

alafanechere commented Dec 7, 2022 •

edited by github-actions bot

Loading

alafanechere left a comment

DoNotPanicUA commented Dec 8, 2022 •

edited by github-actions bot

Loading

		You can find the SQL scripts in this folder if you need to create or fix the SAT dataset.
		For more instructions and information about valid scripts, please check this [doc](https://docs.google.com/document/d/1k5TvxaNhKdr44aJIHWWtLk14Tzd2gbNX-J8YNoTj8u0/edit#heading=h.ls9oiedt9wyy).

Enable full SAT for the Redshift source #19915

Enable full SAT for the Redshift source #19915

Conversation

DoNotPanicUA commented Nov 30, 2022 • edited Loading

What

Configured test instances

Found issues

This comment was marked as outdated.

DoNotPanicUA commented Nov 30, 2022 • edited Loading

✔️ SAT passed. The master branch contains issues in the Status report.

Build Passed

evantahler left a comment • edited Loading

Choose a reason for hiding this comment

evantahler Nov 30, 2022

Choose a reason for hiding this comment

DoNotPanicUA commented Dec 2, 2022 • edited by github-actions bot Loading

Build Passed

DoNotPanicUA commented Dec 6, 2022

alafanechere commented Dec 6, 2022

DoNotPanicUA commented Dec 7, 2022 • edited by github-actions bot Loading

alafanechere Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

DoNotPanicUA Dec 7, 2022

Choose a reason for hiding this comment

evantahler Dec 7, 2022

Choose a reason for hiding this comment

alafanechere commented Dec 7, 2022 • edited by github-actions bot Loading

Build Failed

alafanechere left a comment

Choose a reason for hiding this comment

DoNotPanicUA commented Dec 8, 2022 • edited by github-actions bot Loading

Build Passed

DoNotPanicUA commented Nov 30, 2022 •

edited

Loading

DoNotPanicUA commented Nov 30, 2022 •

edited

Loading

evantahler left a comment •

edited

Loading

DoNotPanicUA commented Dec 2, 2022 •

edited by github-actions bot

Loading

DoNotPanicUA commented Dec 7, 2022 •

edited by github-actions bot

Loading

alafanechere Dec 7, 2022 •

edited

Loading

alafanechere commented Dec 7, 2022 •

edited by github-actions bot

Loading

DoNotPanicUA commented Dec 8, 2022 •

edited by github-actions bot

Loading