Skip to content
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 20357 add missing tap tester tests #53

Open
wants to merge 25 commits into
base: crest-master
Choose a base branch
from

Conversation

NevilParikh14
Copy link

Description of change

Added all the missing integration test

  • Automatic fields test
  • All fields test
  • Discovery test
  • Parent-Child Independent test

Added all the missing assertions

  • Bookmarks test
  • Start date test

Separated Bookmark test for "tickets" and "contacts stream to handle their separate bookmarks and RECORDS collected with the different filter params.

Note: Build will pass after discovery mode is implemented.

Manual QA steps

Risks

Rollback steps

  • revert this branch


@staticmethod
def get_selected_fields_from_metadata(metadata):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add function doc string.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added


from base import FreshdeskBaseTest

# As we are not able to generate following fields by Freshdesk UI, so removed it form expectation list.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# As we are not able to generate following fields by Freshdesk UI, so removed it form expectation list.
# As we are not able to generate following fields by Freshdesk UI, so removed it from expectation list.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Comment on lines 35 to 37
# Tickets and Contacts stream also collectes some deleted data on the basis of filter param.
# Written seprate bookmark test case for them in test_freshdesk_bookmarks_stream_with_fillter_param.py
# To collect "time_entries", "satisfaction_ratings" pro account is needed. Skipping them for now.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Tickets and Contacts stream also collectes some deleted data on the basis of filter param.
# Written seprate bookmark test case for them in test_freshdesk_bookmarks_stream_with_fillter_param.py
# To collect "time_entries", "satisfaction_ratings" pro account is needed. Skipping them for now.
# Tickets and Contacts stream also collect some deleted data on the basis of filter param.
# Written separate bookmark test case for them in test_freshdesk_bookmarks_stream_with_fillter_param.py
# To collect "time_entries", "satisfaction_ratings" pro account is needed. Skipping them for now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Comment on lines 11 to 12
"""Test tap sets a separate bookmark for PUBLISHED and SCHEDULED tweets and
respects it for the next sync of a tweets stream"""

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the above comment, It looks incorrect for this test.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

# Not able to generate more data as roles stream requires pro account.
# So, updating page_sie according to data available.
if stream == "roles":
page_size = 2

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the tap have a default page_size of 2 for the roles stream? If not, then are we passing it using config?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated test case to use configurable 'page_size' param instead of if condition.

@@ -1,170 +1,148 @@
import os

from tap_tester import connections, runner
import requests

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the unused import

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed


from base import FreshdeskBaseTest
from datetime import datetime, timedelta

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this import if not used anywhere

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

# instantiate connection
conn_id_1 = connections.ensure_connection(self)
# Instantiate connection
conn_id_1 = connections.ensure_connection(self, original_properties=False)
Copy link

@namrata270998 namrata270998 Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using the original start_date here, so can we remove the original_properties=False from the ensure_connection?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

tests/base.py Outdated
return {table: properties.get(self.REPLICATION_METHOD, None)
for table, properties
in self.expected_metadata().items()}

def expected_streams(self):
def expected_streams(self, all_streams: bool = False):
"""A set of expected stream names"""

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update this comment

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

"""

# Test for the case of child is selected and parent is not selected
# To collect "time_entries", "satisfaction_ratings" pro account is needed. Skipping them for now.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this comment and add comments for the conversations stream.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the comment.


sync_messages = sync_records.get(stream, {'messages': []}).get('messages')
# To collect "time_entries", "satisfaction_ratings" pro account is needed. Skipping them for now.
expected_streams = streams_to_test

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update comment in all tests. As not excluding here, no need to mention it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

tests/base.py Outdated
def expected_streams(self):
"""A set of expected stream names"""
return set(self.expected_metadata().keys())
def expected_streams(self, all_streams: bool = False):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of all_streams use only_trial_account_stream, take default value False

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@@ -105,7 +105,8 @@ def run_test(self, new_state, stream_to_test, minimum_bookmark, maximum_bookmark
# Verify that the minimum bookmark is used for selected parent-child stream.
replication_key_value = self.dt_to_ts(
record.get(replication_key), self.BOOKMARK_FORMAT)


# Verify that the records replicated for the selected streams are >= given bookmark.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Verify that the records replicated for the selected streams are >= given bookmark.
# Verify that the records replicated for the selected streams are greater than or equal to the given bookmark.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated comment as per suggestion.

tests/base.py Outdated
calculated_state_as_datetime = state_as_datetime - timedelta(days=days, hours=hours, minutes=minutes)

state_format = self.BOOKMARK_FORMAT
calculated_state_formatted = datetime.datetime.strftime(calculated_state_as_datetime, state_format)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of datetime.datetime use dt.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced datetime.datetime with dt.

tests/base.py Outdated
@@ -1,11 +1,12 @@
import os
import unittest
import dateutil.parser
import datetime
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unused datetime package.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed datetime import.

@@ -17,17 +17,16 @@ class FreshdeskBaseTest(unittest.TestCase):
INCREMENTAL = "INCREMENTAL"
FULL = "FULL_TABLE"

START_DATE_FORMAT = "%Y-%m-%dT00:00:00Z" # %H:%M:%SZ
BOOKMARK_FORMAT = "%Y-%m-%dT%H:%M:%SZ"
start_date = ""
Copy link
Member

@sgandhi1311 sgandhi1311 Sep 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is start_date variable with blank string really required?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in the start_date test case we override the start date using this variable.

return "tap_tester_freshdesk_discovery"

def test_run(self):
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we test below scenarios?

  • Verify there are no duplicate/conflicting metadata entries.
  • Verify replication key(s) match expectations.
  • Verify the actual replication matches our expected replication method.
  • Verify that if there is a replication key we are doing INCREMENTAL otherwise FULL.
  • Verify all streams have inclusion of automatic

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the assertions mentioned above are already added to this file.

# Run check mode
# Check mode has no catalog discovery for freshdesk
check_job_name = self.run_and_verify_check_mode(conn_id)
def run_test(self, streams_to_test, page_size):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename the function argument from streams_to_test to expected_streams. And remove expected_streams = streams_to_test from below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated code as per the above suggestion.

Comment on lines 9 to 10
start_date_1 = ""
start_date_2 = ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these 2 lines as those are not needed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already present as an existing code but updated as per your suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants