TDL-17512: Add missing tap-tester tests #134

hpatel41 · 2022-01-31T12:59:45Z

Description of change

TDL-17512: Add missing tap-tester tests

added all fields test
missing assertions in discovery, start date, bookmark test cases.

QA steps

automated tests passing
manual qa steps passing (list below)

Risks

Rollback steps

revert this branch

savan-chovatiya · 2022-02-02T07:23:42Z

tests/base.py

+        # removed "abandoned_checkouts", as per the Doc:
+        #   https://help.shopify.com/en/manual/orders/abandoned-checkouts?st_source=admin&st_campaign=abandoned_checkouts_footer&utm_source=admin&utm_campaign=abandoned_checkouts_footer#review-your-abandoned-checkouts
+        # abandoned checkouts are saved in the Shopify admin for three months.
+        # Every Monday, abandoned checkouts that are older than three months are removed from your admin.
+        return set(self.expected_metadata().keys()) - {"abandoned_checkouts"}


Add comments about the unavailability of the POST call with a reference link.

Added comment in the test cases.

savan-chovatiya · 2022-03-03T10:57:10Z

tests/test_discovery.py

+                    if md_entry['breadcrumb'] != []:
+                        actual_fields.append(md_entry['breadcrumb'][1])
+                # Verify there are no duplicate/conflicting metadata entries.
+                self.assertEqual(len(actual_fields), len(set(actual_fields)), msg="There are duplicate entries in the fields of '{}' stream".format(stream))


Add a comment about this assertion at the function(test_run()) level comment above.

Added function comment.

Good addition

savan-chovatiya · 2022-03-03T10:58:43Z

tests/test_start_date.py

-                target_mark = second_min_bookmarks.get(stream, {"mark": None})
-                target_value = next(iter(target_mark.values()))  # there should be only one
+                # Verify by primary key values, that all records of the 2nd sync are included in the 1st sync since 2nd sync has a later start date.
+                self.assertTrue(primary_keys_sync_2.issubset(primary_keys_sync_1))


Add a comment about this assertion at the function(test_run()) level comment above.

Added function comment.

savan-chovatiya · 2022-03-03T10:59:45Z

tests/test_start_date.py

+                for start_date, target_mark in zip((first_sync_start_date, second_sync_start_date), (first_sync_target_mark, second_sync_target_mark)):
+                    target_value = next(iter(target_mark.values()))  # there should be only one


Add a comment here about the addition of for loop.

Added comment.

dbshah1212 · 2022-03-08T12:18:00Z

tests/test_all_fields.py

+        return return_value
+
+    @staticmethod
+    def get_credentials(original_credentials: bool = True):


It is already available in base.py, Any reason for redefining it?

Updated the test case to use the function from base test file.

tests/test_all_fields.py

tests/test_start_date.py

kspeer825

This is good work. I left one suggestion for the start date test. Also the abandoned_checkouts does not need to be removed. We periodically refresh that data using tooling outside of tap-tester.

kspeer825 · 2022-04-08T12:26:52Z

tests/base.py

-        return set(self.expected_metadata().keys())
+        # removed "abandoned_checkouts", as per the Doc:
+        #   https://help.shopify.com/en/manual/orders/abandoned-checkouts?st_source=admin&st_campaign=abandoned_checkouts_footer&utm_source=admin&utm_campaign=abandoned_checkouts_footer#review-your-abandoned-checkouts
+        # abandoned checkouts are saved in the Shopify admin for three months.
+        # Every Monday, abandoned checkouts that are older than three months are removed from your admin.
+        # Also no POST call is available for this endpoint: https://shopify.dev/api/admin-rest/2022-01/resources/abandoned-checkouts
+        return set(self.expected_metadata().keys()) - {"abandoned_checkouts"}


Please put this stream back, we have internal tooling for generating these records outside of the base test suite.

Included abandoned_checkouts stream

kspeer825 · 2022-04-08T12:27:14Z

tests/base.py

@@ -301,7 +306,12 @@ def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.start_date = self.get_properties().get("start_date")
        self.store_1_streams = {'custom_collections', 'orders', 'products', 'customers', 'locations', 'inventory_levels', 'inventory_items', 'events'}
-        self.store_2_streams = {'abandoned_checkouts', 'collects', 'metafields', 'transactions', 'order_refunds', 'products', 'locations', 'inventory_levels', 'inventory_items', 'events'}
+        # removed 'abandoned_checkouts' from store 2 streams, as per the Doc:


Please put back, see previous comment

Included abandoned_checkouts stream

kspeer825 · 2022-04-08T12:27:58Z

tests/test_automatic_fields.py

                    self.expected_replication_keys().get(stream, set()),
                    msg="The fields sent to the target are not the automatic fields"
                )
+
+                # Verify that all replicated records have unique primary key values.


Good addition

kspeer825 · 2022-04-08T12:29:19Z

tests/test_discovery.py

+                    if md_entry['breadcrumb'] != []:
+                        actual_fields.append(md_entry['breadcrumb'][1])
+                # Verify there are no duplicate/conflicting metadata entries.
+                self.assertEqual(len(actual_fields), len(set(actual_fields)), msg="There are duplicate entries in the fields of '{}' stream".format(stream))


Good addition

kspeer825 · 2022-04-08T12:52:03Z

tests/test_start_date.py

+                first_sync_start_date = self.get_properties()["start_date"]
+                second_sync_start_date = self.start_date
+
+                # loop over minimum bookmark and the start date/state file date for each syncs to verify


The original implementation here was very difficult to follow. These changes are helpful, but can this be updated to conform to current best practices? See https://github.com/singer-io/tap-helpscout/blob/ad2b6192a5f5fad8c5bd0573cb38bf38bd5962dd/tests/test_helpscout_start_date.py#L109-L122

The first 2 assertions are already included in our test case at line no. 141 and 147. Added assertion of replication key-values synced is greater than or equal to the start date.

kspeer825 · 2022-04-11T12:52:29Z

tests/test_start_date.py

+                    for record_replication_key in record_replication_keys:
+                        self.assertGreaterEqual(record_replication_key, start_date)


I think the try except and assertion above are unnecessary. The minimum replication key value is covered by this assertion.

Removed unnecessary assertion.

harshpatel4_crest added 2 commits January 31, 2022 18:28

added missing test cases

b7b0d79

removed abandoned_checkouts from all test cases except discovery

7fd7010

hpatel41 changed the base branch from master to crest-work February 2, 2022 06:03

savan-chovatiya marked this pull request as ready for review February 2, 2022 07:23

savan-chovatiya requested review from savan-chovatiya, dbshah1212 and karanpanchal-crest February 2, 2022 07:23

savan-chovatiya reviewed Feb 2, 2022

View reviewed changes

added comment for abandoned checkouts in test cases

3088df8

hpatel41 requested a review from savan-chovatiya February 2, 2022 09:02

savan-chovatiya suggested changes Mar 3, 2022

View reviewed changes

added code comments and function comments

3daafa7

hpatel41 requested a review from savan-chovatiya March 4, 2022 05:41

dbshah1212 reviewed Mar 8, 2022

View reviewed changes

tests/test_all_fields.py Show resolved Hide resolved

dbshah1212 reviewed Mar 8, 2022

View reviewed changes

tests/test_start_date.py Outdated Show resolved Hide resolved

updated all fields test

273ce0e

hpatel41 requested a review from dbshah1212 March 9, 2022 08:42

savan-chovatiya approved these changes Mar 10, 2022

View reviewed changes

dbshah1212 approved these changes Mar 24, 2022

View reviewed changes

dbshah1212 requested review from KrisPersonal, RushiT0122 and kspeer825 April 7, 2022 05:37

kspeer825 suggested changes Apr 8, 2022

View reviewed changes

include abandoned_checkouts stream and updated start date test case

7bfc94e

hpatel41 requested a review from kspeer825 April 11, 2022 10:23

kspeer825 suggested changes Apr 11, 2022

View reviewed changes

removed unnecessary assertion in start date test

012242e

hpatel41 requested a review from kspeer825 April 11, 2022 13:46

hpatel41 changed the base branch from crest-work to api-sdk-version-change-and-fields-addition April 12, 2022 07:02

hpatel41 changed the base branch from api-sdk-version-change-and-fields-addition to crest-work April 12, 2022 07:02

kspeer825 approved these changes Apr 13, 2022

View reviewed changes

namrata270998 mentioned this pull request Apr 22, 2022

Tdl 15459 update bookmarking for transactions #141

Open

RushiT0122 approved these changes May 6, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TDL-17512: Add missing tap-tester tests #134

TDL-17512: Add missing tap-tester tests #134

hpatel41 commented Jan 31, 2022

savan-chovatiya Feb 2, 2022

hpatel41 Feb 2, 2022

savan-chovatiya Mar 3, 2022

hpatel41 Mar 4, 2022

kspeer825 Apr 8, 2022

savan-chovatiya Mar 3, 2022

hpatel41 Mar 4, 2022

savan-chovatiya Mar 3, 2022

hpatel41 Mar 4, 2022

dbshah1212 Mar 8, 2022

hpatel41 Mar 9, 2022 •

edited

Loading

kspeer825 left a comment

kspeer825 Apr 8, 2022

hpatel41 Apr 11, 2022

kspeer825 Apr 8, 2022

hpatel41 Apr 11, 2022

kspeer825 Apr 8, 2022

kspeer825 Apr 8, 2022

kspeer825 Apr 8, 2022

hpatel41 Apr 11, 2022 •

edited

Loading

kspeer825 Apr 11, 2022

hpatel41 Apr 11, 2022

		for start_date, target_mark in zip((first_sync_start_date, second_sync_start_date), (first_sync_target_mark, second_sync_target_mark)):
		target_value = next(iter(target_mark.values())) # there should be only one

		for record_replication_key in record_replication_keys:
		self.assertGreaterEqual(record_replication_key, start_date)

TDL-17512: Add missing tap-tester tests #134

Are you sure you want to change the base?

TDL-17512: Add missing tap-tester tests #134

Conversation

hpatel41 commented Jan 31, 2022

Description of change

QA steps

Risks

Rollback steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpatel41 Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

kspeer825 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpatel41 Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpatel41 Mar 9, 2022 •

edited

Loading

hpatel41 Apr 11, 2022 •

edited

Loading