Conversation
There was a problem hiding this comment.
Pull request overview
Upgrades the project’s CI environment to run on Python 3.12 and adds a suite of unit + integration tests to validate core tap behaviors (sync, discovery, pagination, bookmarking) under the upgraded runtime.
Changes:
- Update CircleCI to create the virtualenv with Python 3.12 and run unit + integration test suites.
- Add focused unit tests for client pagination/query params, discovery helpers, incremental bookmark logic, and
sync_stream. - Add integration-style tests validating schema discovery metadata, selected-stream sync behavior, bookmark/start-date handling, and pagination behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
.circleci/config.yml |
Switch CI venv to Python 3.12; validate schemas from repo path; add unit & integration test execution steps. |
tests/unittests/test_sync.py |
Unit coverage for sync_stream emitting records and state for incremental streams. |
tests/unittests/test_incremental_sync.py |
Unit coverage for bookmark comparison and update helpers on incremental streams. |
tests/unittests/test_discovery.py |
Unit coverage for schema translation/type inference, merge behavior, and discovery stream enumeration. |
tests/unittests/test_client.py |
Unit coverage for pagination params/URL construction in the client get() method. |
tests/base.py |
Shared integration test utilities (schema loading, record generation, expected metadata). |
tests/test_sync.py |
Integration test ensuring only selected streams are synced and state is written once. |
tests/test_start_date.py |
Integration tests for start-date fallback bookmark behavior and client call argument. |
tests/test_pagination.py |
Integration tests for multi-page reads and stopping on short page. |
tests/test_discovery.py |
Integration test validating expected streams and key replication metadata from discovery output. |
tests/test_bookmark.py |
Integration tests for bookmark reading and incremental bookmark advancement. |
tests/test_automatic_fields.py |
Integration test validating “automatic” inclusion is set for key properties where present. |
tests/test_all_fields.py |
Integration test sanity-checking schema-based record generation across all streams. |
💡 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.
We have chargify test account available please write standard integration tests.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for j in i["invoices"]: | ||
| yield j |
There was a problem hiding this comment.
Chargify.get() determines pagination via len(response.json()) < per_page, which only works when the JSON root is a list. This change makes invoices() expect a dict root (i["invoices"]), so len(response.json()) will be the number of dict keys (likely < 100) and pagination will stop after the first page even when more invoices exist. Consider updating get() to compute page size from the actual records list (e.g., accept a results_key/extractor, or detect dict-vs-list and use the list under the appropriate key) so invoice pagination remains correct.
| for j in i["invoices"]: | |
| yield j | |
| for j in i: | |
| yield j["invoice"] |
There was a problem hiding this comment.
The suggested change breaks at runtime. for j in i on a dict iterates its keys (strings "invoices", "meta"), so j["invoice"] raises TypeError: string indices must be integers.
The current code is correct, unlike other streams where each element is {"customer": {...}} requiring j["customer"], the invoices endpoint returns records directly as plain objects inside the "invoices" array, so yield j (no unwrapping) is right.
There was a problem hiding this comment.
Pages correction made in: 04b543c
| record = {"id": 100, "due_date": "2025-04-20T10:30:00Z"} | ||
| mock_client.invoices.return_value = iter([record]) | ||
|
|
||
| results = list(stream.sync(state)) | ||
|
|
||
| self.assertEqual(len(results), 1) | ||
| self.assertEqual(results[0][1]["due_date"], "2025-04-20T10:30:00Z") |
There was a problem hiding this comment.
The mocked invoice record uses a full timestamp (2025-04-20T10:30:00Z) for due_date, but the tap code/comments and the invoices schema define due_date as a date-only field (format: date, typically YYYY-MM-DD). Aligning the mock data with the actual field shape will make this test more representative and reduce the chance of missing date-only parsing/comparison issues.
| record = {"id": 100, "due_date": "2025-04-20T10:30:00Z"} | |
| mock_client.invoices.return_value = iter([record]) | |
| results = list(stream.sync(state)) | |
| self.assertEqual(len(results), 1) | |
| self.assertEqual(results[0][1]["due_date"], "2025-04-20T10:30:00Z") | |
| record = {"id": 100, "due_date": "2025-04-20"} | |
| mock_client.invoices.return_value = iter([record]) | |
| results = list(stream.sync(state)) | |
| self.assertEqual(len(results), 1) | |
| self.assertEqual(results[0][1]["due_date"], "2025-04-20") |
There was a problem hiding this comment.
Updated due_date with format date-time in invoices.json schema.
Here: 04b543c
Description of change
Ticket: SAC-28693
Manual QA steps
Rollback steps