-
Notifications
You must be signed in to change notification settings - Fork 19
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
TDL-17610: Multiple apps support and data fetching #94
Merged
somethingmorerelevant
merged 23 commits into
master
from
TDL-17610-multiple-apps-support-and-data-fetching
Jul 27, 2023
Merged
Changes from 22 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
521991b
Implement configurable app_ids and wirte test case for them
karanpanchal-crest 07ce0bb
correct message in unit tets
karanpanchal-crest 1685dd4
change README.md file for multiapp selection
karanpanchal-crest 82a1f7d
added logger and exception message in app_ids fetching logic
karanpanchal-crest 8f709c6
Add test cases and correct exception and logger message
karanpanchal-crest f043603
chnage logic for app_ids configuration
karanpanchal-crest aea8de0
correct pylint error
karanpanchal-crest df1bfc9
correct Trailing whitespace error
karanpanchal-crest 83bd358
Add asserts in unittest
karanpanchal-crest 8e8d156
placed invalid app_ids exception inside the main function
karanpanchal-crest f70f51e
add app_ids exception to discovery and sync mode
karanpanchal-crest 93f061c
Add Integration test for the app_ids
karanpanchal-crest a935175
add integration test comments for app_ids
karanpanchal-crest 3cf8ef2
updated filter function
somethingmorerelevant b42f285
Merge branch 'master' into TDL-17610-multiple-apps-support-and-data-f…
somethingmorerelevant c5ae630
updatd filter statement
somethingmorerelevant 470cda8
Merge branch 'master' into TDL-17610-multiple-apps-support-and-data-f…
somethingmorerelevant 4faca9f
Merge branch 'master' into TDL-17610-multiple-apps-support-and-data-f…
somethingmorerelevant a80271b
fixed broken UT
somethingmorerelevant e701b24
Merge branch 'master' into TDL-17610-multiple-apps-support-and-data-f…
somethingmorerelevant ab08c89
fix multi app-ids support to all streams
0c0a1d4
- fix integration test reviews
RushiT0122 f82d5e6
updated readme
somethingmorerelevant File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
import tap_tester.connections as connections | ||
import tap_tester.runner as runner | ||
import tap_tester.menagerie as menagerie | ||
from base import TestPendoBase | ||
|
||
|
||
class PendoMultiAppIdsTest(TestPendoBase): | ||
def name(self): | ||
return "pendo_multi_app_ids_test" | ||
|
||
def test_run(self): | ||
expected_streams = {"features", "pages", "events", "guides", "track_types", "track_events"} | ||
self.run_test(expected_streams=expected_streams, app_ids="-323232", is_multi_apps=False) | ||
self.run_test(expected_streams=expected_streams, app_ids=None, is_multi_apps=True) | ||
self.run_test(expected_streams={"visitors", "visitor_history"}, app_ids=None, is_multi_apps=True) | ||
|
||
def get_properties(self, original: bool = True): | ||
"""Configuration properties required for the tap.""" | ||
if self.streams_to_test == {"visitors", "visitor_history"}: | ||
return_value = { | ||
# To reduce the execution time to test this stream taking recently start_date | ||
"start_date": self.START_DATE_VISTOR_HISTORY, | ||
"lookback_window": "1", | ||
"period": "dayRange", | ||
} | ||
if original: | ||
return return_value | ||
|
||
return return_value | ||
else: | ||
return super().get_properties() | ||
|
||
def run_test(self, expected_streams, app_ids, is_multi_apps, start_date=None): | ||
""" | ||
- Verify tap syncs records for multiple app_ids if no app_ids provided | ||
- Verify tap syncs records for specific app_id if single app_id is provided | ||
""" | ||
|
||
self.start_date = start_date | ||
self.streams_to_test = expected_streams | ||
self.app_ids = app_ids | ||
|
||
conn_id = connections.ensure_connection(self) | ||
|
||
found_catalogs = self.run_and_verify_check_mode(conn_id) | ||
|
||
# table and field selection | ||
test_catalogs_all_fields = [catalog for catalog in found_catalogs | ||
if catalog.get('tap_stream_id') in expected_streams] | ||
|
||
self.perform_and_verify_table_and_field_selection( | ||
conn_id, test_catalogs_all_fields) | ||
|
||
# grab metadata after performing table-and-field selection to set expectations | ||
# used for asserting all fields are replicated | ||
stream_to_all_catalog_fields = dict() | ||
for catalog in test_catalogs_all_fields: | ||
stream_id, stream_name = catalog['stream_id'], catalog['stream_name'] | ||
catalog_entry = menagerie.get_annotated_schema(conn_id, stream_id) | ||
fields_from_field_level_md = [md_entry['breadcrumb'][1] | ||
for md_entry in catalog_entry['metadata'] | ||
if md_entry['breadcrumb'] != []] | ||
stream_to_all_catalog_fields[stream_name] = set( | ||
fields_from_field_level_md) | ||
|
||
self.run_and_verify_sync(conn_id) | ||
|
||
synced_records = runner.get_records_from_target_output() | ||
|
||
for stream in expected_streams: | ||
# below four streams are independent of the app_id | ||
if stream in ["accounts", "visitors", "metadata_accounts", "metadata_visitors"]: | ||
continue | ||
|
||
with self.subTest(stream=stream): | ||
records_appid_set = set([message.get('data').get('app_id') for message in synced_records.get(stream).get("messages")]) | ||
self.assertEqual(len(records_appid_set)>1, is_multi_apps) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Are the visitors and visitor_history the only streams that support this test? I'm leaning towards using other streams here if possible. The logic being that this suite may be cumbersome to maintain since it's noted that data for these streams must be manually generated every 3 months. It would be nice not to lose all coverage and have the whole suite fail when that occurs and just have certain relevant tests fail (all streams, all fields, etc).
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.
Only "accounts", "visitors", "metadata_accounts" and "metadata_visitors" streams are not supported for this test and to ensure maximum test coverage within the three-hour maximum execution time in cci, we handle
visitor_history
stream in a specific manner. Note earlier we used to skip this stream due same limitation.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.
Ok