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

CDK: remove unexpected error swallowing on abstract source's check method #24240

Merged
merged 19 commits into from
Mar 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
ec30d74
cdk: do not handle Exceptions in check
alafanechere Mar 20, 2023
70e704f
cdk: do not handle Exceptions in check
alafanechere Mar 20, 2023
467d7cf
cat: assert trace message on exception
alafanechere Mar 20, 2023
7173644
source-airtable: new exception test case for check
alafanechere Mar 20, 2023
993df8a
bump CDK version
alafanechere Mar 20, 2023
578561a
bump CAT version
alafanechere Mar 20, 2023
12abe30
Merge branch 'master' into augustin/remove-unexpected-error-swallowing
alafanechere Mar 21, 2023
5456d1a
Merge branch 'master' into augustin/remove-unexpected-error-swallowing
alafanechere Mar 21, 2023
664a6be
remove assertion on internal_message existence
alafanechere Mar 21, 2023
2672fb7
Merge branch 'master' into augustin/remove-unexpected-error-swallowing
alafanechere Mar 22, 2023
cc843db
revert changelog
alafanechere Mar 22, 2023
517143e
update source-paypal acceptance test config
alafanechere Mar 23, 2023
834cb28
update source-google-analytics-v4 acceptance test config
alafanechere Mar 23, 2023
de1cdc3
update source-zendesk-talk acceptance test config
alafanechere Mar 23, 2023
b5f45b6
update source-amazon-ads acceptance test config
alafanechere Mar 23, 2023
cb600c3
update source-google-ads acceptance test config
alafanechere Mar 23, 2023
1422bb6
update source-pinterest acceptance test config
alafanechere Mar 23, 2023
42afb20
update source-marketo acceptance test config
alafanechere Mar 23, 2023
2b0be99
Merge branch 'master' into augustin/remove-unexpected-error-swallowing
alafanechere Mar 23, 2023
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
10 changes: 3 additions & 7 deletions airbyte-cdk/python/airbyte_cdk/sources/abstract_source.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,13 +79,9 @@ def check(self, logger: logging.Logger, config: Mapping[str, Any]) -> AirbyteCon
"""Implements the Check Connection operation from the Airbyte Specification.
See https://docs.airbyte.com/understanding-airbyte/airbyte-protocol/#check.
"""
try:
check_succeeded, error = self.check_connection(logger, config)
if not check_succeeded:
return AirbyteConnectionStatus(status=Status.FAILED, message=repr(error))
except Exception as e:
return AirbyteConnectionStatus(status=Status.FAILED, message=repr(e))

check_succeeded, error = self.check_connection(logger, config)
if not check_succeeded:
return AirbyteConnectionStatus(status=Status.FAILED, message=repr(error))
return AirbyteConnectionStatus(status=Status.SUCCEEDED)

def read(
Expand Down
14 changes: 6 additions & 8 deletions airbyte-cdk/python/unit_tests/sources/test_abstract_source.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,11 @@ def test_failed_check():
assert expected == MockSource(check_lambda=lambda: (False, "womp womp")).check(logger, {})


def test_raising_check():
def test_raising_check(mocker):
"""Tests that if a source raises an unexpected exception the appropriate connectionStatus failure message is returned."""
expected = AirbyteConnectionStatus(status=Status.FAILED, message="Exception('this should fail')")
assert expected == MockSource(check_lambda=lambda: exec('raise Exception("this should fail")')).check(logger, {})
check_lambda = mocker.Mock(side_effect=BaseException("this should fail"))
with pytest.raises(BaseException):
MockSource(check_lambda=check_lambda).check(logger, {})


class MockStream(Stream):
Expand Down Expand Up @@ -334,10 +335,7 @@ def test_valid_full_refresh_read_with_slices(mocker):

@pytest.mark.parametrize(
"slices",
[
[{"1": "1"}, {"2": "2"}],
[{"date": datetime.date(year=2023, month=1, day=1)}, {"date": datetime.date(year=2023, month=1, day=1)}]
]
[[{"1": "1"}, {"2": "2"}], [{"date": datetime.date(year=2023, month=1, day=1)}, {"date": datetime.date(year=2023, month=1, day=1)}]],
)
def test_read_full_refresh_with_slices_sends_slice_messages(mocker, slices):
"""Given the logger is debug and a full refresh, AirbyteMessages are sent for slices"""
Expand Down Expand Up @@ -369,7 +367,7 @@ def test_read_incremental_with_slices_sends_slice_messages(mocker):
debug_logger.setLevel(logging.DEBUG)
slices = [{"1": "1"}, {"2": "2"}]
stream = MockStream(
[({"sync_mode": SyncMode.incremental, "stream_slice": s, 'stream_state': {}}, [s]) for s in slices],
[({"sync_mode": SyncMode.incremental, "stream_slice": s, "stream_state": {}}, [s]) for s in slices],
name="s1",
)

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Changelog

## 0.7.2
TestConnection: assert that a check with `exception` status emits a trace message.

Comment on lines +3 to +5
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if this should be bumped differently, as it's a new test we expect some to fail 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only two connectors are using status: "exception" in their acceptance test config (dixa and file). They both are not passing CAT at the moment.
I don't expect additional connectors to fail as only these connectors + airtable will go into the inputs.status == ConnectionTestConfig.Status.Exception: statement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, thanks for clarifying 👍🏻

## 0.7.1
Discovery backward compatibility tests: handle errors on previous connectors catalog retrieval. Return None when the discovery failed. It should unblock the situation when tests fails even if you bypassed backward compatibility tests.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ COPY pytest.ini setup.py ./
COPY connector_acceptance_test ./connector_acceptance_test
RUN pip install .

LABEL io.airbyte.version=0.7.1
LABEL io.airbyte.version=0.7.2
LABEL io.airbyte.name=airbyte/connector-acceptance-test

ENTRYPOINT ["python", "-m", "pytest", "-p", "connector_acceptance_test.plugin", "-r", "fEsx"]
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from airbyte_cdk.models import (
AirbyteRecordMessage,
AirbyteStream,
AirbyteTraceMessage,
ConfiguredAirbyteCatalog,
ConfiguredAirbyteStream,
ConnectorSpecification,
Expand Down Expand Up @@ -46,7 +47,6 @@
find_keyword_schema,
)
from connector_acceptance_test.utils.json_schema_helper import JsonSchemaHelper, get_expected_schema_structure, get_object_structure
from docker.errors import ContainerError
from jsonschema._utils import flatten


Expand Down Expand Up @@ -488,11 +488,13 @@ def test_check(self, connector_config, inputs: ConnectionTestConfig, docker_runn
assert len(con_messages) == 1, "Connection status message should be emitted exactly once"
assert con_messages[0].connectionStatus.status == Status.FAILED
elif inputs.status == ConnectionTestConfig.Status.Exception:
with pytest.raises(ContainerError) as err:
docker_runner.call_check(config=connector_config)

assert err.value.exit_status != 0, "Connector should exit with error code"
assert "Traceback" in err.value.stderr, "Connector should print exception"
output = docker_runner.call_check(config=connector_config, raise_container_error=False)
trace_messages = filter_output(output, Type.TRACE)
assert len(trace_messages) == 1, "A trace message should be emitted in case of unexpected errors"
trace = trace_messages[0].trace
assert isinstance(trace, AirbyteTraceMessage)
assert trace.error is not None
assert trace.error.message is not None


@pytest.mark.default_timeout(30)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ acceptance_tests:
- config_path: "secrets/config_oauth.json"
status: "succeed"
- config_path: "integration_tests/invalid_config_oauth.json"
status: "failed"
status: "failed"
- config_path: "integration_tests/invalid_config_oauth_missing_fields.json"
status: "exception"
discovery:
tests:
- config_path: "secrets/config.json"
Expand All @@ -31,7 +33,7 @@ acceptance_tests:
path: "integration_tests/expected_records.jsonl"
extra_fields: true
exact_order: true
extra_records: false
extra_records: false
- config_path: "secrets/config_oauth.json"
expect_records:
path: "integration_tests/expected_records.jsonl"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"credentials": {
"auth_method": "oauth2.0",
"client_id": "client_id",
"client_secret": "client_secret",
"refresh_token": "refresh_token",
"token_expiry_date": "2023-01-01T12:12:12.000000+00:00"
}
}
Original file line number Diff line number Diff line change
@@ -1,62 +1,62 @@
acceptance_tests:
basic_read:
tests:
- config_path: secrets/config.json
empty_streams:
- name: sponsored_brands_ad_groups
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: sponsored_brands_campaigns
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: sponsored_brands_keywords
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: attribution_report_performance_creative
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: attribution_report_performance_adgroup
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: attribution_report_products
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: attribution_report_performance_campaign
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: sponsored_display_report_stream
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: sponsored_brands_report_stream
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: sponsored_brands_video_report_stream
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: sponsored_products_report_stream
bypass_reason: "can't populate stream because it requires real ad campaign"
timeout_seconds: 2400
expect_records:
path: integration_tests/expected_records.jsonl
- config_path: secrets/config.json
empty_streams:
- name: sponsored_brands_ad_groups
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: sponsored_brands_campaigns
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: sponsored_brands_keywords
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: attribution_report_performance_creative
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: attribution_report_performance_adgroup
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: attribution_report_products
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: attribution_report_performance_campaign
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: sponsored_display_report_stream
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: sponsored_brands_report_stream
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: sponsored_brands_video_report_stream
bypass_reason: "can't populate stream because it requires real ad campaign"
- name: sponsored_products_report_stream
bypass_reason: "can't populate stream because it requires real ad campaign"
timeout_seconds: 2400
expect_records:
path: integration_tests/expected_records.jsonl
connection:
tests:
- config_path: secrets/config.json
status: succeed
- config_path: integration_tests/invalid_config.json
status: failed
- config_path: secrets/config.json
status: succeed
- config_path: integration_tests/invalid_config.json
status: exception
discovery:
tests:
- config_path: secrets/config.json
- config_path: secrets/config.json
full_refresh:
tests:
- config_path: secrets/config.json
configured_catalog_path: integration_tests/configured_catalog.json
- config_path: secrets/config_report.json
configured_catalog_path: integration_tests/configured_catalog_report.json
timeout_seconds: 3600
- config_path: secrets/config.json
configured_catalog_path: integration_tests/configured_catalog.json
- config_path: secrets/config_report.json
configured_catalog_path: integration_tests/configured_catalog_report.json
timeout_seconds: 3600
incremental:
tests:
- config_path: secrets/config_report.json
configured_catalog_path: integration_tests/configured_catalog_report.json
cursor_paths:
sponsored_products_report_stream:
- '1861552880916640'
- reportDate
future_state:
future_state_path: integration_tests/abnormal_state.json
timeout_seconds: 2400
- config_path: secrets/config_report.json
configured_catalog_path: integration_tests/configured_catalog_report.json
cursor_paths:
sponsored_products_report_stream:
- "1861552880916640"
- reportDate
future_state:
future_state_path: integration_tests/abnormal_state.json
timeout_seconds: 2400
spec:
tests:
- spec_path: integration_tests/spec.json
- spec_path: integration_tests/spec.json
connector_image: airbyte/source-amazon-ads:dev
test_strictness_level: high
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ acceptance_tests:
- config_path: "secrets/config.json"
status: "succeed"
- config_path: "integration_tests/invalid_config.json"
status: "failed"
status: "exception"
discovery:
tests:
- config_path: "secrets/config.json"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,37 +1,37 @@
acceptance_tests:
basic_read:
tests:
- config_path: secrets/service_config.json
empty_streams:
- name: users_per_city
bypass_reason: no records in the stream
expect_records:
path: integration_tests/expected_records.jsonl
timeout_seconds: 1800
- config_path: secrets/service_config.json
empty_streams:
- name: users_per_city
bypass_reason: no records in the stream
expect_records:
path: integration_tests/expected_records.jsonl
timeout_seconds: 1800
connection:
tests:
- config_path: secrets/service_config.json
status: succeed
- config_path: secrets/old_config.json
status: succeed
- config_path: integration_tests/invalid_config.json
status: failed
- config_path: secrets/service_config.json
status: succeed
- config_path: secrets/old_config.json
status: succeed
- config_path: integration_tests/invalid_config.json
status: exception
discovery:
tests:
- config_path: secrets/service_config.json
- config_path: secrets/service_config.json
full_refresh:
tests:
- config_path: secrets/service_config.json
configured_catalog_path: integration_tests/configured_catalog.json
- config_path: secrets/service_config.json
configured_catalog_path: integration_tests/configured_catalog.json
incremental:
tests:
- config_path: secrets/service_config.json
configured_catalog_path: integration_tests/configured_catalog.json
future_state:
future_state_path: integration_tests/abnormal_state.json
threshold_days: 2
- config_path: secrets/service_config.json
configured_catalog_path: integration_tests/configured_catalog.json
future_state:
future_state_path: integration_tests/abnormal_state.json
threshold_days: 2
spec:
tests:
- spec_path: source_google_analytics_v4/spec.json
- spec_path: source_google_analytics_v4/spec.json
connector_image: airbyte/source-google-analytics-v4:dev
test_strictness_level: high
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ acceptance_tests:
- config_path: "secrets/config.json"
status: "succeed"
- config_path: "integration_tests/invalid_config.json"
status: "failed"
status: "exception"
discovery:
tests:
- config_path: "secrets/config.json"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,36 +9,36 @@ acceptance_tests:
- config_path: secrets/config.json
status: succeed
- config_path: integration_tests/invalid_config.json
status: failed
status: exception
- config_path: secrets/config_oauth.json
status: succeed
- config_path: integration_tests/invalid_config_oauth.json
status: failed
status: exception
discovery:
tests:
- config_path: secrets/config.json
- config_path: secrets/config.json
basic_read:
tests:
- config_path: secrets/config.json
empty_streams:
- name: balances
bypass_reason: "value of 'last_refresh_time' field changes during every read"
timeout_seconds: 1200
expect_records:
path: "integration_tests/expected_records.jsonl"
extra_fields: no
exact_order: no
extra_records: yes
- config_path: secrets/config.json
empty_streams:
- name: balances
bypass_reason: "value of 'last_refresh_time' field changes during every read"
timeout_seconds: 1200
expect_records:
path: "integration_tests/expected_records.jsonl"
extra_fields: no
exact_order: no
extra_records: yes
incremental:
tests:
- config_path: secrets/config.json
configured_catalog_path: integration_tests/configured_catalog.json
future_state:
future_state_path: integration_tests/abnormal_state.json
cursor_paths:
transactions: [ "date" ]
balances: [ "date" ]
- config_path: secrets/config.json
configured_catalog_path: integration_tests/configured_catalog.json
future_state:
future_state_path: integration_tests/abnormal_state.json
cursor_paths:
transactions: ["date"]
balances: ["date"]
full_refresh:
tests:
- config_path: secrets/config.json
configured_catalog_path: integration_tests/configured_catalog.json
- config_path: secrets/config.json
configured_catalog_path: integration_tests/configured_catalog.json
Loading