-
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
TDL-17610: Multiple apps support and data fetching #94
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.
@karanpanchal-crest CCi job has failed, please look into it.
d0df678
to
a935175
Compare
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.
reviewed and tested these changes
bdab5d0
to
ab08c89
Compare
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) |
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
@bhtowles please re-review. |
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) |
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
Description of change
Manual QA steps
Risks
Rollback steps