-
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
source freshdesk, hubspot, mixpanel, paypal-transaction, salesforce, zendesk-support: adding fixtures to mock time.sleep for connectors that explicitly sleep #12335
Conversation
/test connector=connectors/source-freshdesk
|
/test connector=connectors/source-salesforce
|
/test connector=connectors/source-hubspot
|
/test connector=connectors/source-mixpanel
|
/test connector=connectors/source-paypal-transaction
|
/test connector=connectors/source-zendesk-support
|
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 great! Let's update the changelogs (eg https://github.com/airbytehq/airbyte/blob/master/docs/integrations/sources/zendesk-support.md?plain=1#L73), then publish the connectors and bump the versions (https://docs.airbyte.com/connector-development/#updating-an-existing-connector)
Codecov Report
@@ Coverage Diff @@
## master #12335 +/- ##
=========================================
Coverage ? 80.52%
=========================================
Files ? 24
Lines ? 2470
Branches ? 0
=========================================
Hits ? 1989
Misses ? 481
Partials ? 0 Continue to review full report at Codecov.
|
/publish connector=connectors/freshdesk
|
/publish connector=connectors/source-freshdesk
|
/publish connector=connectors/source-hubspot
|
/publish connector=connectors/source-mixpanel
|
/publish connector=connectors/source-salesforce
|
/publish connector=connectors/source-zendesk-support
|
/publish connector=connectors/source-paypal-transaction
|
…m/airbytehq/airbyte into brian/mock_sleep_to_speedup_tests
/publish connector=connectors/source-mixpanel
|
…m/airbytehq/airbyte into brian/mock_sleep_to_speedup_tests
/publish connector=connectors/source-mixpanel
|
/publish connector=connectors/source-mixpanel
|
/publish connector=connectors/source-mixpanel
|
…zendesk-support: adding fixtures to mock time.sleep for connectors that explicitly sleep (#12335) * adding fixtures to mock time.sleep for connectors that explicitly sleep * bump connector versions * update changelog doc for each connector * auto-bump connector version * auto-bump connector version * auto-bump connector version * auto-bump connector version * auto-bump connector version * remove version bump for freshdesk because connector tests are in a bad state Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
What
Addresses #11784
We have plenty of unit tests that spend a bulk of their time sleeping because of a backoff for retryable errors or because of custom connector logic. Since unit tests are run in isolation, we don't need to waste our time waiting.
Connectors changed
How
As a first pass I specifically addressed connectors that had custom
time.sleep()
calls within their code. The concept is pretty straightforward that ifpytest-mock
isn't imported then I added it for a given connector. And then I introduced a new test fixture that mockstime.sleep()
and that fixture is set to be auto used for each test in that file.Note:
time.sleep()
is used in the default behavior for theuser_defined_backoff_handler
. I'm certain that other connectors are also unnecessarily sleeping in unit tests, but I haven't figured out a great way to fix them all at once. My proposition is in the immediate term as part of the maintenance process for connectors, if long running connector unit tests are identified, then we emulate the behavior here to mocktime.sleep()
on a case by case basis. But potentially we might want to explore the concept of test configs that will instrument this and other test primitives by default, but still offering a way to enable/disable.Recommended reading order
This change affects a couple of connectors, order of reading doesn't matter really.
🚨 User Impact 🚨
Only changes to tests, no breaking changes
Pre-merge Checklist
Expand the relevant checklist and delete the others.
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereTests
Unit
Integration
Integration test run results can be seen in the comments.
freshdesk
has existing failuresAcceptance
Put your acceptance tests output here.