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: cause AirbyteTraceMessage by using invalid sync mode #12960

Merged
merged 4 commits into from
May 18, 2022

Conversation

pedroslopez
Copy link
Contributor

@pedroslopez pedroslopez commented May 18, 2022

What

The test case introduced in #12796 assumed that connectors will fail/crash if given a non-existent stream name as part of the configured catalog. However, this was not necessarily the case (for example, google sheets does not do this and instead exits silently). We should make this fail more reliably.

How

In addition to supplying an invalid stream name, also supply invalid sync modes so validation should not pass.

@pedroslopez

This comment was marked as resolved.

@pedroslopez

This comment was marked as resolved.

@pedroslopez
Copy link
Contributor Author

pedroslopez commented May 18, 2022

/test connector=source-google-sheets

🕑 source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/2346598289
❌ source-google-sheets https://github.com/airbytehq/airbyte/actions/runs/2346598289
🐛 https://gradle.com/s/dzq5uwng7ip6w
Python short test summary info:

=========================== short test summary info ============================
FAILED test_core.py::TestBasicRead::test_airbyte_trace_message_on_failure[inputs0]
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config
============= 1 failed, 22 passed, 1 skipped in 127.92s (0:02:07) ==============

@pedroslopez
Copy link
Contributor Author

pedroslopez commented May 18, 2022

Looks like the /test commands pull the latest SAT from docker hub - here are the results from running acctests locally:

 test_core.py ✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓                                                                                                                                                                                      96% █████████▋
 test_full_refresh.py ✓                                                                                                                                                                                                  100% ██████████

======================================================================================================= short test summary info ========================================================================================================
SKIPPED [1] source_acceptance_test/plugin.py:56: Skipping TestIncremental.test_two_sequential_reads because not found in the config

Results (142.05s):
      23 passed

And for stripe:

 test_core.py ✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓✓                                                                                                                                                                             82% ████████▎ 
 test_full_refresh.py ✓✓✓                                                                                                                                                                                                 89% ████████▉ 
 test_incremental.py ✓✓✓✓                                                                                                                                                                                                100% ██████████


Results (609.63s):
      38 passed

@pedroslopez
Copy link
Contributor Author

pedroslopez commented May 18, 2022

/test connector=source-facebook-marketing

🕑 source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/2347514101
❌ source-facebook-marketing https://github.com/airbytehq/airbyte/actions/runs/2347514101
🐛 https://gradle.com/s/gcw3dvg26a7je
Python short test summary info:

=========================== short test summary info ============================
FAILED test_full_refresh.py::TestFullRefresh::test_sequential_reads[inputs0]
FAILED test_incremental.py::TestIncremental::test_read_sequential_slices[inputs0]
================== 2 failed, 22 passed in 1981.49s (0:33:01) ===================

@pedroslopez pedroslopez marked this pull request as ready for review May 18, 2022 19:02
@pedroslopez
Copy link
Contributor Author

source-facebook-marketing failure is unrelated to the new test case

@pedroslopez pedroslopez requested review from evantahler and sherifnada and removed request for evantahler May 18, 2022 19:23
Comment on lines +428 to +429
sync_mode="INVALID",
destination_sync_mode="INVALID",
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, those are invalid alight!

@pedroslopez
Copy link
Contributor Author

pedroslopez commented May 18, 2022

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

🕑 bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/2348029654
🚀 Successfully published bases/source-acceptance-test
✅ bases/source-acceptance-test https://github.com/airbytehq/airbyte/actions/runs/2348029654

@pedroslopez pedroslopez merged commit 838af6f into master May 18, 2022
@pedroslopez pedroslopez deleted the pedroslopez/fix-sat-trace branch May 18, 2022 20:40
suhomud pushed a commit that referenced this pull request May 23, 2022
* cause failure by using invalid sync mode

* create configured stream without passing pydantic validation

* fix formatting

* bump version, update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants