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

SAT: unit tests test_state_with_abnormally_large_values + add missing future_state_path #17791

Merged
merged 7 commits into from
Oct 12, 2022

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Oct 10, 2022

What

Closes #17338
Just a sanity check over our "source should always emit state" testing.

How

  • Unit test the test_state_with_abnormally_large_values to make sure it works as expected
  • Add missing future_state_path to acceptance-test-config.yml of the following connectors:
    • source-surveymonkey [GA]
    • source-bigcommerce [alpha]
    • source-talkdesk-explore [alpha]
    • source-paystack [alpha]

The other connector missing future_state_path testing are the following:

  • source-amplitude [GA]
  • source-snapchat-marketing [GA]
  • source-zendesk-chat [GA]
  • source-aws-cloudtrail [alpha]
  • source-strava [alpha]

It's not possible to check these connector always emit a state when no record is available because their API does not allow querying future data, which is what test_state_with_abnormally_large_values is testing.

@github-actions
Copy link
Contributor

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-chargebee
  • source-asana
  • source-sentry
  • source-gitlab
  • source-braintree
  • source-pinterest
  • source-okta
  • source-lemlist
  • source-google-ads
  • source-cart
  • source-zenloop
  • source-openweather
  • source-notion
  • source-freshcaller
  • source-delighted
  • source-freshsales
  • source-azure-table
  • source-strava
  • source-harvest
  • source-lever-hiring
  • source-klaviyo
  • source-pipedrive
  • source-retently
  • source-commercetools
  • source-instagram
  • source-youtube-analytics
  • source-salesforce
  • source-onesignal
  • source-plaid
  • source-facebook-marketing
  • source-appsflyer
  • source-google-search-console
  • source-amplitude
  • source-amazon-ads
  • source-facebook-pages
  • source-outreach
  • source-posthog
  • source-recharge
  • source-prestashop
  • source-github
  • source-drift
  • source-surveymonkey
  • source-greenhouse
  • source-linnworks
  • source-confluence
  • source-airtable
  • source-sendgrid
  • source-zendesk-talk
  • source-amazon-sqs
  • source-zendesk-sunshine
  • source-iterable
  • source-twilio
  • source-pardot
  • source-mailgun
  • source-paystack
  • source-amazon-seller-partner
  • source-salesloft
  • source-mailchimp
  • source-monday
  • source-freshservice
  • source-tplcentral
  • source-quickbooks-singer

@alafanechere alafanechere marked this pull request as ready for review October 10, 2022 15:04
@alafanechere
Copy link
Contributor Author

alafanechere commented Oct 10, 2022

/test connector=connectors/source-surveymonkey

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

Name                              Stmts   Miss  Cover
-----------------------------------------------------
source_surveymonkey/__init__.py       2      0   100%
source_surveymonkey/streams.py      131      7    95%
source_surveymonkey/source.py        49     13    73%
-----------------------------------------------------
TOTAL                               182     20    89%
	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          10      4    60%   15-18
	 source_acceptance_test/config.py                        83      6    93%   78-80, 84-86
	 source_acceptance_test/conftest.py                     164    164     0%   6-282
	 source_acceptance_test/plugin.py                        48     48     0%   6-104
	 source_acceptance_test/tests/test_core.py              329    111    66%   39, 50-58, 63-70, 74-75, 79-80, 164, 202-219, 228-236, 240-245, 251, 284-289, 327-334, 374-376, 379, 439-448, 477-478, 484, 487, 520-530, 543-568, 573-577
	 source_acceptance_test/tests/test_full_refresh.py       52      2    96%   34, 65
	 source_acceptance_test/tests/test_incremental.py       152     20    87%   21-23, 29-31, 36-43, 48-61, 239
	 source_acceptance_test/utils/asserts.py                 37      2    95%   57-58
	 source_acceptance_test/utils/common.py                  77     10    87%   15-16, 24-30, 64, 67
	 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-67, 70-72, 75-77, 80-82, 85-87, 90-92, 95-113, 147-149
	 source_acceptance_test/utils/json_schema_helper.py     105     13    88%   30-31, 38, 41, 65-68, 96, 120, 190-192
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1358    453    67%

Build Passed

Test summary info:

All Passed

@alafanechere
Copy link
Contributor Author

alafanechere commented Oct 10, 2022

/test connector=connectors/source-bigcommerce

🕑 connectors/source-bigcommerce https://github.com/airbytehq/airbyte/actions/runs/3220347370
❌ connectors/source-bigcommerce https://github.com/airbytehq/airbyte/actions/runs/3220347370
🐛 https://gradle.com/s/nmt3wsu3c7za6

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_incremental.py::TestIncremental::test_state_with_abnormally_large_values[inputs0]
======================== 1 failed, 28 passed in 56.66s =========================

@alafanechere
Copy link
Contributor Author

alafanechere commented Oct 10, 2022

/test connector=connectors/source-talkdesk-explore

🕑 connectors/source-talkdesk-explore https://github.com/airbytehq/airbyte/actions/runs/3220350062
❌ connectors/source-talkdesk-explore https://github.com/airbytehq/airbyte/actions/runs/3220350062
🐛 https://gradle.com/s/ygd3rxbsk27oo

Build Failed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:60: Skipping TestFullRefresh.test_sequential_reads because not found in the config
============================== 1 skipped in 1.32s ==============================

@alafanechere
Copy link
Contributor Author

alafanechere commented Oct 10, 2022

/test connector=connectors/source-paystack

🕑 connectors/source-paystack https://github.com/airbytehq/airbyte/actions/runs/3220350074
❌ connectors/source-paystack https://github.com/airbytehq/airbyte/actions/runs/3220350074
🐛 https://gradle.com/s/k5gecf34dn3o2

Build Failed

Test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestSpec::test_additional_properties_is_true[inputs0] - ...
FAILED test_core.py::TestBasicRead::test_read[inputs0] - Failed: Please check...
=================== 2 failed, 27 passed in 81.46s (0:01:21) ====================

@github-actions
Copy link
Contributor

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-gitlab
  • source-linnworks
  • source-youtube-analytics
  • source-zendesk-sunshine
  • source-freshcaller
  • source-cart
  • source-chargebee
  • source-delighted
  • source-pipedrive
  • source-sentry
  • source-salesforce
  • source-freshsales
  • source-appsflyer
  • source-freshservice
  • source-recharge
  • source-amplitude
  • source-sendgrid
  • source-drift
  • source-google-search-console
  • source-lever-hiring
  • source-greenhouse
  • source-salesloft
  • source-strava
  • source-google-ads
  • source-instagram
  • source-facebook-pages
  • source-github
  • source-facebook-marketing
  • source-lemlist
  • source-amazon-sqs
  • source-confluence
  • source-retently
  • source-amazon-ads
  • source-zenloop
  • source-commercetools
  • source-mailgun
  • source-okta
  • source-monday
  • source-paystack
  • source-onesignal
  • source-zendesk-talk
  • source-tplcentral
  • source-braintree
  • source-posthog
  • source-amazon-seller-partner
  • source-harvest
  • source-azure-table
  • source-quickbooks-singer
  • source-asana
  • source-prestashop
  • source-pinterest
  • source-iterable
  • source-notion
  • source-twilio
  • source-surveymonkey
  • source-mailchimp
  • source-airtable
  • source-klaviyo
  • source-pardot
  • source-openweather
  • source-outreach
  • source-plaid

@alafanechere
Copy link
Contributor Author

I don't expect SAT to pass on alpha connectors are they are also failing on master...

Copy link
Contributor

@evantahler evantahler left a comment

Choose a reason for hiding this comment

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

👍

A question about "The other connector missing future_state_path testing are the following"

  • did you audit all the connectors? How did you come up with this list?

A question about: "I don't expect SAT to pass on alpha connectors are they are also failing on master..."

  • How did you learn this? I wonder if we should keep a spreadsheet or metabase report about this to help connector developers know what to do if they need to re-publish a connector

@@ -18,7 +18,7 @@ tests:
incremental:
- config_path: "secrets/config.json"
configured_catalog_path: "integration_tests/configured_catalog.json"
# future_state_path: "integration_tests/abnormal_state.json"
future_state_path: "integration_tests/abnormal_state.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

I love to see more tests being enabled!

@alafanechere
Copy link
Contributor Author

did you audit all the connectors? How did you come up with this list?

Quick script parsing the acceptance-test-config.yml files of each connector declaring incremental test. Declared missing if future_state_path is not set.
I'd like to suggest a permanent approach at this coverage evaluation in the tech spec of #17531

A question about: "I don't expect SAT to pass on alpha connectors are they are also failing on master..."
How did you learn this? I wonder if we should keep a spreadsheet or metabase report about this to help connector developers know what to do if they need to re-publish a connector

https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/builds.md

@alafanechere
Copy link
Contributor Author

alafanechere commented Oct 11, 2022

/publish connector=bases/source-acceptance-test auto-bump-version=false

🕑 Publishing the following connectors:
bases/source-acceptance-test
https://github.com/airbytehq/airbyte/actions/runs/3226018106


Connector Did it publish? Were definitions generated?
bases/source-acceptance-test

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@alafanechere
Copy link
Contributor Author

alafanechere commented Oct 11, 2022

/publish connector=bases/source-acceptance-test auto-bump-version=false

🕑 Publishing the following connectors:
bases/source-acceptance-test
https://github.com/airbytehq/airbyte/actions/runs/3228970392


Connector Did it publish? Were definitions generated?
bases/source-acceptance-test

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@github-actions
Copy link
Contributor

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-paystack
  • source-github
  • source-confluence
  • source-notion
  • source-harvest
  • source-mailgun
  • source-gitlab
  • source-azure-table
  • source-pardot
  • source-zendesk-sunshine
  • source-asana
  • source-klaviyo
  • source-braintree
  • source-tplcentral
  • source-sentry
  • source-amazon-ads
  • source-instagram
  • source-airtable
  • source-zendesk-talk
  • source-okta
  • source-iterable
  • source-recharge
  • source-zenloop
  • source-prestashop
  • source-pinterest
  • source-youtube-analytics
  • source-drift
  • source-delighted
  • source-pipedrive
  • source-onesignal
  • source-lever-hiring
  • source-freshcaller
  • source-openweather
  • source-retently
  • source-posthog
  • source-salesforce
  • source-twilio
  • source-amazon-seller-partner
  • source-commercetools
  • source-plaid
  • source-freshsales
  • source-cart
  • source-salesloft
  • source-amplitude
  • source-facebook-marketing
  • source-lemlist
  • source-facebook-pages
  • source-appsflyer
  • source-freshservice
  • source-mailchimp
  • source-chargebee
  • source-google-search-console
  • source-outreach
  • source-quickbooks-singer
  • source-surveymonkey
  • source-linnworks
  • source-amazon-sqs
  • source-monday
  • source-strava
  • source-google-ads
  • source-sendgrid
  • source-greenhouse

@alafanechere
Copy link
Contributor Author

alafanechere commented Oct 12, 2022

/publish connector=bases/source-acceptance-test auto-bump-version=false

🕑 Publishing the following connectors:
bases/source-acceptance-test
https://github.com/airbytehq/airbyte/actions/runs/3233879029


Connector Did it publish? Were definitions generated?
bases/source-acceptance-test

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@github-actions
Copy link
Contributor

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-salesforce
  • source-zendesk-talk
  • source-braintree
  • source-posthog
  • source-facebook-marketing
  • source-asana
  • source-linnworks
  • source-pardot
  • source-amazon-sqs
  • source-confluence
  • source-drift
  • source-amazon-seller-partner
  • source-notion
  • source-sentry
  • source-mailgun
  • source-twilio
  • source-tplcentral
  • source-amplitude
  • source-quickbooks-singer
  • source-google-ads
  • source-youtube-analytics
  • source-instagram
  • source-recharge
  • source-klaviyo
  • source-plaid
  • source-outreach
  • source-freshsales
  • source-retently
  • source-github
  • source-freshservice
  • source-okta
  • source-amazon-ads
  • source-zenloop
  • source-chargebee
  • source-commercetools
  • source-pinterest
  • source-google-search-console
  • source-paystack
  • source-facebook-pages
  • source-appsflyer
  • source-mailchimp
  • source-cart
  • source-harvest
  • source-salesloft
  • source-sendgrid
  • source-zendesk-sunshine
  • source-lemlist
  • source-prestashop
  • source-greenhouse
  • source-delighted
  • source-pipedrive
  • source-airtable
  • source-monday
  • source-gitlab
  • source-strava
  • source-azure-table
  • source-lever-hiring
  • source-surveymonkey
  • source-openweather
  • source-freshcaller
  • source-iterable
  • source-onesignal

@alafanechere
Copy link
Contributor Author

alafanechere commented Oct 12, 2022

/publish connector=bases/source-acceptance-test auto-bump-version=false

🕑 Publishing the following connectors:
bases/source-acceptance-test
https://github.com/airbytehq/airbyte/actions/runs/3236002361


Connector Did it publish? Were definitions generated?
bases/source-acceptance-test

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@github-actions
Copy link
Contributor

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to run corresponding integration tests:

  • source-confluence
  • source-klaviyo
  • source-salesforce
  • source-iterable
  • source-pardot
  • source-freshsales
  • source-airtable
  • source-pinterest
  • source-appsflyer
  • source-zenloop
  • source-salesloft
  • source-sendgrid
  • source-surveymonkey
  • source-amazon-seller-partner
  • source-greenhouse
  • source-google-search-console
  • source-paystack
  • source-asana
  • source-delighted
  • source-lemlist
  • source-linnworks
  • source-facebook-pages
  • source-sentry
  • source-notion
  • source-posthog
  • source-lever-hiring
  • source-cart
  • source-strava
  • source-youtube-analytics
  • source-facebook-marketing
  • source-mailgun
  • source-chargebee
  • source-zendesk-sunshine
  • source-prestashop
  • source-mailchimp
  • source-outreach
  • source-commercetools
  • source-monday
  • source-drift
  • source-freshcaller
  • source-twilio
  • source-amazon-ads
  • source-braintree
  • source-gitlab
  • source-google-ads
  • source-onesignal
  • source-tplcentral
  • source-plaid
  • source-retently
  • source-quickbooks-singer
  • source-azure-table
  • source-harvest
  • source-recharge
  • source-freshservice
  • source-amazon-sqs
  • source-zendesk-talk
  • source-okta
  • source-pipedrive
  • source-amplitude
  • source-github
  • source-openweather
  • source-instagram

@alafanechere alafanechere merged commit 0cbd4de into master Oct 12, 2022
@alafanechere alafanechere deleted the augustin/sat/allways-emit-state branch October 12, 2022 17:32
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SAT verifies that an incremental stream with no data still produces a state message
3 participants