-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
airbyte-lib: Add testing to connectors #34044
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
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 code looks good to me and I like the fact that the validation happens in the airbyte-lib package.
I'm a bit more concern by the footprint of this new check on the connector tests in CI.
Could we make the following part of the validation optional to not overlap the test with CAT parts?
* Answers according to the Airbyte protocol when called with spec, check, discover and read.
@alafanechere Maybe this is worded badly - what it means to say is that airbyte-lib can use the connector as expected, which includes getting the spec to statically validate the config the user has provided, running check to fully validate it, and running discover and read to actually fetch data and give it to the user. The check is not doing any protocol-specific checks, it's just trying to use airbyte-lib with the connector:
When you say footprint, are you talking about test runtimes? Because this is relatively lightweight in this sense, it calls spec, check, discover and read only once and stops after the very first record. The reason why I would like to actually call read in this test is that some connectors do things only if a read is actually triggered (e.g. lazily loading some modules or static files) and I want to validate that this works correctly. If airbyte-lib managed to get a record from a single stream out of the connector, there is a very good chance the other ones will work too |
…ib-testing-proper
This makes sense to me, and I'm 🆗 with it if it's not significantly slowing down the full test suite. One concern remains: calling read or discover on a connector with short lived access token means these operation can lead to an update of the access token that has to be reflected on the GSM side. Is your validation code performing this update? |
@alafanechere good point, I changed this PR to only implement the basic check of "install works and spec can be fetched" for now. I will create an issue to track the more complete setup |
The failing test looks unrelated to the changes to me (and also passes locally) - maybe it's flaky? |
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.
Thanks for the change.
Would you mind manually testing it on a couple of connectors by dispatching this workflow?
…bytehq/airbyte into flash1293/airbyte-lib-testing-proper
Ran tests for a bunch of connectors (see above) and the airbyte-lib validation passed for all of them, merging and crafting a message to connector devs so they are aware |
Building on the
airbyte-lib-validate-source
command to validate whether a python connector works with airbyte-lib, this PR introduces a new optional test step to run this command with the current connector under test.To test:
make tools.airbyte-ci-dev.install
airbyte-ci connectors --name=source-apify-dataset test