TDL-7596: Add tap-tester test for attribution window #169

hpatel41 · 2021-10-07T14:07:25Z

Description of change

TDL-7596: Add tap-tester test for attribution window

Added test case for testing 7 day and 28 day attribution window.

NOTE: The UI does not allow the attribution window to be set except {1, 7, 28}. The tap is also working when we set an attribution window other than this.

Manual QA steps

Risks

Rollback steps

revert this branch

dbshah1212 · 2021-11-12T08:41:21Z

tests/test_facebook_attribution_window.py

+class FacebookAttributionWindow(FacebookBaseTest):
+
+    # set attribution window
+    attrubution_window = 7


Define constants in caps like ATTRIBUTION_WINDOW

dbshah1212 · 2021-11-12T08:42:25Z

tests/test_facebook_attribution_window.py

+    def name():
+        return "tap_tester_facebook_attribution_window"
+
+    def get_properties(self, original: bool = True):


Why do we need this method here, isn't it defined in the base.py?

Yes, it is defined, but here we wanted to use different values of attribution window.

Then can't we add one more optional parameter?

No, here only 1 argument can be passed in the function.

This is an unfortunate restriction of the tap-tester backend.

karanpanchal-crest · 2021-11-17T11:36:08Z

tests/test_facebook_attribution_window.py

+                    replication_key_value = record.get(replication_key)
+
+                    # Verify the sync records respect the (simulated) start date value
+                    self.assertGreaterEqual(self.parse_date(replication_key_value), self.parse_date(start_date_with_attribution_window),


@harshpatel4crest Can you please explain how does the 3 checks asked in the DoD of this ticket is accumulated in this test case?

self.assertGreaterEqual(self.parse_date(replication_key_value), self.parse_date(start_date_with_attribution_window) is for the 1st check.

self.assertTrue(is_between, msg="No record found between start date and attribution date.") is for the 2nd check, where is_between is the boolean value for the checking if any record is present between the start date and buffered start date.

3rd check is about the error message when we pass the attribution window other than {1, 7, 28}. But the UI does not allow the attribution window to be set except {1, 7, 28}. The tap is also working when we set an attribution window other than this.

Please complete the 3rd assertion. If the tap does not throw an acceptable error, that is a bug and should be documented and linked in the test with a workaround.

Facebook only supports those values, so even though the tap does not blow up when given a different value I think we should force an error so that we match what they claim they support. Additionally we have use cases where the tap is ran directly so, just because the UI has restrictions around the values does not mean a user will not send something crazy into the config.

FB Docs: https://www.facebook.com/business/help/395050428485124?id=428636648170202

Stitch Docs: https://www.stitchdata.com/docs/integrations/saas/facebook-ads#select-attribution-window

@kspeer825 Added code change to raise an error when the attribution window is not 1, 7 or 28.

kspeer825 · 2021-11-18T12:34:06Z

tests/test_facebook_attribution_window.py

+                    replication_key_value = record.get(replication_key)
+
+                    # Verify the sync records respect the (simulated) start date value
+                    self.assertGreaterEqual(self.parse_date(replication_key_value), self.parse_date(start_date_with_attribution_window),


Please complete the 3rd assertion. If the tap does not throw an acceptable error, that is a bug and should be documented and linked in the test with a workaround.

kspeer825 · 2021-11-18T12:34:34Z

tests/test_facebook_attribution_window.py

+                    replication_key_value = record.get(replication_key)
+
+                    # Verify the sync records respect the (simulated) start date value
+                    self.assertGreaterEqual(self.parse_date(replication_key_value), self.parse_date(start_date_with_attribution_window),


Facebook only supports those values, so even though the tap does not blow up when given a different value I think we should force an error so that we match what they claim they support. Additionally we have use cases where the tap is ran directly so, just because the UI has restrictions around the values does not mean a user will not send something crazy into the config.

FB Docs: https://www.facebook.com/business/help/395050428485124?id=428636648170202

Stitch Docs: https://www.stitchdata.com/docs/integrations/saas/facebook-ads#select-attribution-window

kspeer825 · 2021-11-18T13:55:25Z

tests/unittests/test_attribution_window.py

+import unittest
+import tap_facebook.__init__ as tap_facebook
+
+class TestAttributionWindow(unittest.TestCase):


It's fine to leave in this unittest, but this should be a tap-tester case. We want to prove the error is thrown during discovery.

@kspeer825 Added integration test for verifying error is thrown in discovery mode.

* TDL-9728: Stream `ads_insights_age_gender` has unexpected datatype for replication key field `date_start` (#172) * added format as date-time in schema file * added code coverage * added check for date format in the bookmark test * added the check for first sync messages Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> * TDL-9809: `forced-replication-method` missing from metadata for some streams and TDL-9872: replication keys are not specified as expected in discoverable metadata (#167) * added valid replication keys in catalog * modified the code * TDL-9809: Added replication keys in metadata * adde code coverage * Resolved review comments Co-authored-by: harshpatel4_crest <harsh.patel4@crestdatasys.com> Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> * TDL-7455: Add tap-tester test to verify replication of deleted records (#168) * TDL-7455: Added archived data integration test * TDL-7455: Updated integration test * added code coverage * Resolved review comment Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> * TDL-7596: Add tap-tester test for attribution window (#169) * added tap tester test for attribution window * updated the code * added code coverage * updated the code according to the comments * updated code to raise error when attribution window is not 1, 7, 28 * test: run invalid attribution window intergation test * updated test case * test: updated test case code * test: test invalid attribution window * test: test invalid attribution window * test: test invalid attribution window * test: test invalid attribution window * test: run invalid attribution window test case * added intergation test for invalid sttribution window Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> Co-authored-by: savan-chovatiya <80703490+savan-chovatiya@users.noreply.github.com>

* TDL-9728: Stream `ads_insights_age_gender` has unexpected datatype for replication key field `date_start` (singer-io#172) * added format as date-time in schema file * added code coverage * added check for date format in the bookmark test * added the check for first sync messages Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> * TDL-9809: `forced-replication-method` missing from metadata for some streams and TDL-9872: replication keys are not specified as expected in discoverable metadata (singer-io#167) * added valid replication keys in catalog * modified the code * TDL-9809: Added replication keys in metadata * adde code coverage * Resolved review comments Co-authored-by: harshpatel4_crest <harsh.patel4@crestdatasys.com> Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> * TDL-7455: Add tap-tester test to verify replication of deleted records (singer-io#168) * TDL-7455: Added archived data integration test * TDL-7455: Updated integration test * added code coverage * Resolved review comment Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> * TDL-7596: Add tap-tester test for attribution window (singer-io#169) * added tap tester test for attribution window * updated the code * added code coverage * updated the code according to the comments * updated code to raise error when attribution window is not 1, 7, 28 * test: run invalid attribution window intergation test * updated test case * test: updated test case code * test: test invalid attribution window * test: test invalid attribution window * test: test invalid attribution window * test: test invalid attribution window * test: run invalid attribution window test case * added intergation test for invalid sttribution window Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> Co-authored-by: namrata270998 <namrata.brahmbhatt@crestdatasys.com> Co-authored-by: savan-chovatiya <80703490+savan-chovatiya@users.noreply.github.com>

added tap tester test for attribution window

295ab53

hpatel41 changed the base branch from master to crest-master October 11, 2021 04:40

harshpatel4_crest and others added 2 commits October 11, 2021 17:55

updated the code

484c41a

added code coverage

fec4797

hpatel41 marked this pull request as ready for review November 10, 2021 13:37

dbshah1212 reviewed Nov 12, 2021

View reviewed changes

updated the code according to the comments

655f945

karanpanchal-crest reviewed Nov 17, 2021

View reviewed changes

hpatel41 requested a review from karanpanchal-crest November 17, 2021 12:20

dbshah1212 self-requested a review November 17, 2021 13:15

dbshah1212 approved these changes Nov 17, 2021

View reviewed changes

dbshah1212 requested review from kspeer825, KrisPersonal and manand31 November 17, 2021 13:16

kpanchal-crest approved these changes Nov 17, 2021

View reviewed changes

karanpanchal-crest approved these changes Nov 17, 2021

View reviewed changes

KrisPersonal approved these changes Nov 17, 2021

View reviewed changes

kspeer825 suggested changes Nov 18, 2021

View reviewed changes

updated code to raise error when attribution window is not 1, 7, 28

1bf6960

hpatel41 requested a review from kspeer825 November 18, 2021 13:49

kspeer825 suggested changes Nov 18, 2021

View reviewed changes

harshpatel4_crest added 8 commits November 19, 2021 14:18

test: run invalid attribution window intergation test

a698010

updated test case

23a7e4a

test: updated test case code

0fa3b4c

test: test invalid attribution window

46b2832

test: test invalid attribution window

2edfbc8

test: test invalid attribution window

fae3e78

test: test invalid attribution window

d21d8d0

test: run invalid attribution window test case

713d9e0

harshpatel4_crest added 2 commits November 19, 2021 15:02

added intergation test for invalid sttribution window

cc24a23

resolve merge conflicts and updated config.yml file

064d3af

hpatel41 requested a review from kspeer825 November 19, 2021 11:27

kspeer825 approved these changes Nov 19, 2021

View reviewed changes

hpatel41 merged commit 923761b into crest-master Nov 19, 2021

hpatel41 mentioned this pull request Nov 19, 2021

Crest Work #176

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TDL-7596: Add tap-tester test for attribution window #169

TDL-7596: Add tap-tester test for attribution window #169

hpatel41 commented Oct 7, 2021 •

edited

Loading

dbshah1212 Nov 12, 2021

hpatel41 Nov 17, 2021

dbshah1212 Nov 12, 2021

hpatel41 Nov 12, 2021

dbshah1212 Nov 17, 2021

hpatel41 Nov 17, 2021

dbshah1212 Nov 17, 2021

kspeer825 Nov 18, 2021

karanpanchal-crest Nov 17, 2021

hpatel41 Nov 17, 2021 •

edited

Loading

kspeer825 Nov 18, 2021

kspeer825 Nov 18, 2021

hpatel41 Nov 18, 2021

kspeer825 Nov 18, 2021

kspeer825 Nov 18, 2021

kspeer825 Nov 18, 2021

hpatel41 Nov 19, 2021

TDL-7596: Add tap-tester test for attribution window #169

TDL-7596: Add tap-tester test for attribution window #169

Conversation

hpatel41 commented Oct 7, 2021 • edited Loading

Description of change

Manual 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

hpatel41 Nov 17, 2021 • edited Loading

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 commented Oct 7, 2021 •

edited

Loading

hpatel41 Nov 17, 2021 •

edited

Loading