-
Notifications
You must be signed in to change notification settings - Fork 2
SAC 28711: Update libraries and integration tests #31
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
base: SAC-28849-add-new-metadata
Are you sure you want to change the base?
SAC 28711: Update libraries and integration tests #31
Conversation
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.
Pull request overview
This PR updates core library dependencies and modifies the backoff retry strategy from a constant interval approach to an exponential backoff pattern, while also fixing various integration test issues including date formatting inconsistencies and expanding test data coverage.
Changes:
- Updated
singer-pythonfrom 5.13.2 to 6.3.0 andrequestsfrom 2.32.4 to 2.32.5 - Changed backoff strategy from constant 30-second intervals with 5 retries to exponential backoff with 7 retries
- Fixed integration test date formats, updated test dates to 2024-2025, and added missing fields for stream validation
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.py | Updates library versions for singer-python and requests |
| tap_iterable/iterable.py | Changes backoff strategy from constant to exponential with updated retry configuration |
| tests/unittests/test_backoff.py | Updates expected retry counts from 5 to 7 to match new backoff configuration |
| tests/base.py | Updates start date, adds email_bounce to skipped streams, and fixes state format to include time |
| tests/test_start_date.py | Updates test date from 2023 to 2024 and fixes start_date property access |
| tests/test_interrupted_sync.py | Updates bookmark dates to 2025 and standardizes datetime format with T separator |
| tests/test_all_fields.py | Adds numerous missing fields for validation across multiple streams and updates start date to 2020 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
RushiT0122
left a comment
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.
Observed that last bookmarked record is missing from next sync for templates stream. Please reverify the implementation.
RushiT0122
left a comment
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.
Observed that last bookmarked record is missing from next sync for templates stream. Please reverify the implementation and update the bookmark tests accordingly.
As per discussion, I have updated the sync logic for |
| # We are moving with pseudo-incremental logic here to avoid missing out records due to API limitations. | ||
| # ## Approach: | ||
| # 1) Get the bookmark value from state. | ||
| # 2) Fetch all records from the API without passing any bookmark/startDateTime filter. Basic FULL_TABLE logic. | ||
| # 3) For each record fetched, check the record's updatedAt value with bookmark value from state. | ||
| # 4) If record's updatedAt >= bookmark value from state, yield the record | ||
| # 5) Save the max updatedAt value from the fetched records as new bookmark in state. | ||
|
|
||
| # Reason for the workaround: | ||
| # ------------------------- |
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 part is not required since it just talks about what we are doing which already coded there. We should just focus on why we need this workaround.
| # We are moving with pseudo-incremental logic here to avoid missing out records due to API limitations. | |
| # ## Approach: | |
| # 1) Get the bookmark value from state. | |
| # 2) Fetch all records from the API without passing any bookmark/startDateTime filter. Basic FULL_TABLE logic. | |
| # 3) For each record fetched, check the record's updatedAt value with bookmark value from state. | |
| # 4) If record's updatedAt >= bookmark value from state, yield the record | |
| # 5) Save the max updatedAt value from the fetched records as new bookmark in state. | |
| # Reason for the workaround: | |
| # ------------------------- | |
| # We are moving with pseudo-incremental logic here to avoid missing out records due to API limitations. | |
| # ## Approach: | |
| # 1) Get the bookmark value from state. | |
| # 2) Fetch all records from the API without passing any bookmark/startDateTime filter. Basic FULL_TABLE logic. | |
| # 3) For each record fetched, check the record's updatedAt value with bookmark value from state. | |
| # 4) If record's updatedAt >= bookmark value from state, yield the record | |
| # 5) Save the max updatedAt value from the fetched records as new bookmark in state. | |
| # Reason for the workaround: | |
| # ------------------------- |
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.
Got it. Changes are pushed
…tps://github.com/singer-io/tap-iterable into SAC-28711/update-libraries-and-integration-tests
|
@satyendra101 : Needs to update config.yml to use python 3.12 |
Description of change
Manual QA steps
Risks
Rollback steps
AI generated code
https://internal.qlik.dev/general/ways-of-working/code-reviews/#guidelines-for-ai-generated-code