-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
SAT: check that connectors emit an AirbyteTraceMessage
on failure
#12796
Conversation
streams=[ | ||
ConfiguredAirbyteStream( | ||
stream=AirbyteStream( | ||
name="__AIRBYTE__stream_that_does_not_exist", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test assumes that connectors should always crash if we're trying to sync a stream name that does not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me!
Are there any other types of failures we might want to check? What about running with an invalid config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't speak to the pythonic-ness of this PR, but logically this looks great! I think there might be other types of failures to check for the trace message (invalid config, a random SIGKILL), but those can be added in later PRs.
@@ -144,6 +156,95 @@ def test_read(schema, record, should_fail): | |||
t.test_read(None, catalog, input_config, [], docker_runner_mock, MagicMock()) | |||
|
|||
|
|||
@pytest.mark.parametrize( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a test mock I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parametrize
is pretty neat - it lets you easily run the same test with different input configurations. https://docs.pytest.org/en/6.2.x/parametrize.html
streams=[ | ||
ConfiguredAirbyteStream( | ||
stream=AirbyteStream( | ||
name="__AIRBYTE__stream_that_does_not_exist", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me!
Are there any other types of failures we might want to check? What about running with an invalid config?
/publish connector=bases/source-acceptance-test auto-bump-version=false
|
…12796) * add SAT case for airbyte trace message on failure * add ability to opt-out * add tests * add option to docs * bump version, update changelog * fix type errors * update changelog
What
Verify that connectors emit at least one
AirbyteTraceMessage
on crash.closes #12473
How
AirbyteTraceMessage
is presentRecommended reading order
tests/test_core.py
unit_tests/test_core.py
🚨 User Impact 🚨
Connectors will no longer pass the SAT if they do not emit an
AirbyteTraceMessage
on generic failures. For most cases, this should just mean updating to the latest version of the CDK (>= 0.1.56).If the connector really shouldn't emit an airbyte trace message, it can opt out of the test by setting
expect_trace_message_on_failure=False
on thebasic_read
test config.Tests
Unit