Conversation
There was a problem hiding this comment.
Pull request overview
Adds automated test coverage (unit + tap-tester style integration tests) for the tap_frontapp tap, while also aligning emitted analytics_date values with a date-time schema and bumping dependency versions.
Changes:
- Add new unit test suite for
schemas,discover,context, and__init__entrypoint behaviors. - Add tap-tester integration test scaffolding (
tests/base.py+ standard suite tests). - Update
analytics_datetodate-timein schemas and emitanalytics_dateas an ISO-8601 UTC timestamp; bump package/dependency versions and update changelog.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unittests/test_schemas.py | Unit tests for schema loading/normalization and Singer metadata construction. |
| tests/unittests/test_init.py | Unit tests for tap_frontapp entrypoint helpers and main() behavior. |
| tests/unittests/test_discovery.py | Unit tests for discovery/catalog creation and credential validation logic. |
| tests/unittests/test_context.py | Unit tests for Context initialization, catalog selection behavior, and bookmark/state helpers. |
| tests/unittests/init.py | Marks tests/unittests as a package. |
| tests/base.py | Base class and expected stream metadata for tap-tester integration tests. |
| tests/test_discovery.py | Tap-tester discovery conformance test hookup. |
| tests/test_all_fields.py | Tap-tester “all fields replicated” test hookup. |
| tests/test_automatic_fields.py | Tap-tester “automatic fields replicated” test hookup. |
| tests/test_bookmark.py | Tap-tester bookmark behavior test hookup. |
| tests/test_interrupted_sync.py | Tap-tester interrupted sync/resume test hookup. |
| tests/test_start_date.py | Tap-tester start-date behavior test hookup. |
| tests/test_pagination.py | Tap-tester pagination test hookup (currently excluding all streams). |
| tap_frontapp/streams.py | Emit analytics_date as YYYY-MM-DDT00:00:00Z instead of date-only. |
| tap_frontapp/schemas/accounts_table.json | Mark analytics_date with format: date-time. |
| tap_frontapp/schemas/channels_table.json | Mark analytics_date with format: date-time. |
| tap_frontapp/schemas/inboxes_table.json | Mark analytics_date with format: date-time. |
| tap_frontapp/schemas/tags_table.json | Mark analytics_date with format: date-time. |
| tap_frontapp/schemas/teammates_table.json | Mark analytics_date with format: date-time. |
| tap_frontapp/schemas/teams_table.json | Mark analytics_date with format: date-time. |
| setup.py | Bump package version and dependency pins. |
| CHANGELOG.md | Add 2.3.0 entry documenting dependency upgrades. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_discover_raises_on_invalid_schema(self, mock_get_schemas): | ||
| """Test that discover succeeds with a lenient schema dict.""" | ||
| from singer import metadata as md | ||
| bad_mdata = md.new() | ||
| bad_mdata = md.write(bad_mdata, (), "table-key-properties", []) | ||
| mock_get_schemas.return_value = ( | ||
| {"bad_stream": {"type": "invalid_type_that_should_still_load"}}, | ||
| {"bad_stream": bad_mdata}, | ||
| ) | ||
|
|
||
| catalog = discover() | ||
| self.assertEqual(len(catalog.streams), 1) | ||
|
|
There was a problem hiding this comment.
test_discover_raises_on_invalid_schema name/docstring indicate an exception is expected, but the test asserts discover() succeeds and returns a 1-stream catalog. Please rename/reword this test (and its docstring) to match the intended behavior (e.g., "succeeds" vs "raises"), or change the assertions to actually expect an exception if that's what should happen.
| bookmark_format = "%Y-%m-%dT%H:%M:%SZ" | ||
| initial_bookmarks = { | ||
| "bookmarks": { | ||
| "accounts_table": {"date_to_resume": "2020-01-01 00:00:00"}, | ||
| "channels_table": {"date_to_resume": "2020-01-01 00:00:00"}, | ||
| "inboxes_table": {"date_to_resume": "2020-01-01 00:00:00"}, | ||
| "tags_table": {"date_to_resume": "2020-01-01 00:00:00"}, | ||
| "teammates_table": {"date_to_resume": "2020-01-01 00:00:00"}, | ||
| "teams_table": {"date_to_resume": "2020-01-01 00:00:00"}, | ||
| } |
There was a problem hiding this comment.
bookmark_format is set to an ISO-8601 "...T...Z" pattern, but initial_bookmarks uses values like "2020-01-01 00:00:00" (space-separated). The tap currently writes bookmarks via to_datetime_string() ("YYYY-MM-DD HH:MM:SS"), so this mismatch is likely to break the bookmark integration test. Align bookmark_format and/or initial_bookmarks with the actual state value format the tap produces.
| from tap_tester.base_suite_tests.bookmark_test import BookmarkTest | ||
|
|
||
|
|
||
| class FrontAppBookMarkTest(BookmarkTest, FrontAppBaseTest): |
There was a problem hiding this comment.
Class name FrontAppBookMarkTest has inconsistent capitalization ("BookMark") compared to the other test classes in this suite (e.g., FrontAppStartDateTest, FrontAppPaginationTest). Consider renaming to FrontAppBookmarkTest for consistency and easier discovery/searching.
| class FrontAppBookMarkTest(BookmarkTest, FrontAppBaseTest): | |
| class FrontAppBookmarkTest(BookmarkTest, FrontAppBaseTest): |
Description of change
SAC-28717
Manual QA steps
Risks
Rollback steps
AI generated code
https://internal.qlik.dev/general/ways-of-working/code-reviews/#guidelines-for-ai-generated-code