-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: Add validation of data types in the output records #3253 #4345
Conversation
add `validate_schema` config option, On by default
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.
LGTM! Just a few small comments.
## 0.1.6 | ||
Add configurable validation of schema for all records in BasicRead test: https://github.com/airbytehq/airbyte/pull/4345 | ||
The validation is ON by default. | ||
To disable validation for the source you need to set `validate_schema: off` in the config file. |
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.
Perhaps validate_schema: false
?
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 think this should be on, so we will introduce this aggressively and then fix tests for specific connectors. In this way, we will ensure that new connectors follow best practices from the begining.
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.
if you mean that off
should be false
instead, then it is pydantic feature to encode boolean as true, 1, on, yes...
https://pydantic-docs.helpmanual.io/usage/types/#booleans
airbyte-integrations/bases/source-acceptance-test/source_acceptance_test/tests/test_core.py
Outdated
Show resolved
Hide resolved
def test_required(self): | ||
"""Check that connector will fail if any required field is missing""" | ||
|
||
def test_optional(self): | ||
"""Check that connector can work without any optional field""" | ||
|
||
def test_has_secret(self): | ||
"""Check that spec has a secret. Not sure if this should be always the case""" | ||
|
||
def test_secret_never_in_the_output(self): | ||
"""This test should be injected into any docker command it needs to know current config and spec""" | ||
|
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.
Do we need these empty tests in this PR?
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 don't think it is gonna hurt, just documented future tests. I've used tests to document the idea, instead of documenting this as part of the ticket.
…tance_test/tests/test_core.py Co-authored-by: Vadym <vege1wgw@gmail.com>
from jsonschema import Draft4Validator, ValidationError | ||
|
||
|
||
def verify_records_schema( |
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.
we need to start doing this at some point so no time like the present...
could we write a unit test for this functionality?
also pls make sure to update the PR title and formatting |
/publish connector=base/source-acceptance-test
|
/publish connector=bases/source-acceptance-test
|
What
added new config option
validate_schema
tobasic_read
test (ON by default).How
Describe the solution