TDL-14989: Check best practices #66

hpatel41 · 2021-08-24T07:36:49Z

Description of change

TDL-14989: Check best practices

Updated the tap-tester image tap-tester-v4 -> stitch-tap-tester
Updated the sandbox -> tap_tester_sandbox
Updated singer-python 5.9.0 -> 5.12.1
Added Integration tests for:
- All fields
- Bookmark
- Sync
NOTE:
- On upgrading to singer-python 5.12.1, SortImports error was coming ,as per the discussion on updating pylint version, the error was resolved, but another error occurred posonlyargs and as per the discussion , updating the astroid version got it resolved.
- Updated exit with sys.exit because pylint was failing.
- Updated the super used in the code with Python 3 style super() without arguments as pylint was filing by using super with arguments.

Manual QA steps

Risks

Rollback steps

revert this branch

dbshah1212 · 2021-08-26T09:42:08Z

tests/test_exacttarget_all_fields.py

+            with self.subTest(stream=stream):
+
+                # # expected values
+                # expected_automatic_keys = self.expected_automatic_fields().get(stream)


Why we commented this line? Can we remove the line if not required?

dbshah1212 · 2021-08-26T09:44:34Z

tests/test_exacttarget_all_fields.py

+                    expected_all_keys.remove('SendClassification') # not retrievable
+                    expected_all_keys.remove('PartnerProperties') # not retrievable
+                elif stream == 'subscriber':
+                    expected_all_keys.remove('CustomerKey') # not retrievable


Create a list and run a loop for this.

dbshah1212 · 2021-08-30T11:56:17Z

tests/test_exacttarget_bookmarks.py

+        ### Update Start Date
+        ##########################################################################
+
+        self.START_DATE = '2021-01-01T00:00:00Z'


For the Bookmark test why we are updating start_date? We need to update the state.

Updated the code change to use state.

dbshah1212 · 2021-08-30T12:01:12Z

tests/base.py

+        return {
+            "campaign": {
+                "primary-key": {"id"},
+                "replication": "FULL"


Instead of using primary-key, replication, replication-key, FULL, INCREMENTAL, create constants of those and use those. use FULL_TABLE instead of FULL

Updated the code.

dbshah1212 · 2021-08-31T09:23:41Z

.circleci/config.yml

@@ -21,21 +21,15 @@ jobs:
          name: 'pylint'
          command: |
            source /usr/local/share/virtualenvs/tap-exacttarget/bin/activate
-            pylint tap_exacttarget -d no-else-return,wrong-import-order,pointless-string-statement,invalid-name,deprecated-method,bad-whitespace,line-too-long
+            pylint tap_exacttarget -d no-else-return,wrong-import-order,pointless-string-statement,invalid-name,deprecated-method,bad-whitespace,line-too-long,consider-using-sys-exit


Why do we need to update the pylint?

Added a NOTE about that in the PR description.

dbshah1212 · 2021-08-31T09:27:56Z

setup.py

        'python-dateutil==2.6.0',
        'voluptuous==0.10.5',
        'Salesforce-FuelSDK==1.3.0'
    ],
    extras_require={
        'dev': [
            'ipdb==0.11',
-            'pylint==2.1.1',
-            'astroid==2.1.0',
+            'pylint==2.4.4',


Any special requirement to upgrade pylint and astroid?

@harshpatel4crest Please update both the modules to their latest versions currently available on pypi

kspeer825 · 2021-09-17T10:31:56Z

tests/test_exacttarget_discover.py

@@ -1,101 +0,0 @@
-import datetime


Instead of removing the discovery test, could you update it to meet current standards https://github.com/stitchdata/tap-tester/blob/master/reference/expectations/saas_taps.md#test_discoverypy

Yes, it is added as part of PR #68.

kspeer825 · 2021-09-17T10:33:40Z

setup.py

        'dev': [
            'ipdb==0.11',
-            'pylint==2.1.1',
-            'astroid==2.1.0',
+            'pylint==2.10.2',
+            'astroid==2.7.3',


Can you actually separate these into a two groups: dev and test. The test group should have astroid and nose and we should install that group as part of the circleci build.

Updated the code.

kspeer825 · 2021-09-17T10:40:17Z

tests/test_exacttarget_bookmarks.py

+            if stream == "subscriber":
+                continue
+


Can you elaborate on why this stream cannot be tested. If we cannot get a replication key value from the api, we should consider making this stream Full Table.

I am still not clear on why this stream cannot be tested here.

Subscriber (child stream) will not have any bookmark as it only syncs the provided list of subscribers keys and the subscribers key will be created from the list_subscribers (parent stream) based on the records returned from the API.

As discussed here

tests/base.py

…nResetErrors (#71) * added backoff for certain errors * resolve pylint * updated decorator location * added unittests * added comment * added comments

* updated error message when generating auth_stub * made changes according to the comments * updated the code acording to the comments * updated the tap tester image * updated pylint and astroid to latest version * updated the code as, on updating tap tester image it was throwing cascading errors * updated config.yml file * updated the start date for integration tests as per the params * removed scenario as new tap-tester version does not support it * updated start date in the base file test

kspeer825

Can this PR be closed, it is mentioned in a different already merged PR.

dbshah1212 · 2021-11-16T06:19:34Z

Can this PR be closed, it is mentioned in a different already merged PR.

@KrisPersonal will you close these kinds of PRs? As we are merging one master(crest-master) branch only.

harshpatel4_crest added 3 commits August 24, 2021 12:58

added best practices

b09b817

resolve pylint

366bf45

resolve test failure

8b8059c

hpatel41 changed the title ~~Added best practices~~ TDL-14989: Check best practices Aug 24, 2021

harshpatel4_crest added 2 commits August 24, 2021 16:06

test: updated the test cases

2909f30

test: updated some test cases

e1b895d

dbshah1212 reviewed Aug 26, 2021

View reviewed changes

harshpatel4_crest added 2 commits August 27, 2021 12:43

updated the code as per comments

e93808f

resolve test case failure

9b0ddeb

hpatel41 requested a review from dbshah1212 August 27, 2021 08:31

hpatel41 marked this pull request as ready for review August 27, 2021 09:33

hpatel41 requested review from karanpanchal-crest and savan-chovatiya August 27, 2021 09:33

dbshah1212 reviewed Aug 30, 2021

View reviewed changes

dbshah1212 reviewed Aug 31, 2021

View reviewed changes

harshpatel4_crest added 12 commits September 1, 2021 13:58

updated the code as per comments

80d4431

resolve integration test failure

9fa65ef

resolve tes case failure

f8a2906

resolve test case failure

6b0f8f3

add data extension stream in test cases

a0a8959

added data extension incremental stream

368f864

test: run bookmark test

dabd289

test: run bookmark test

f368415

test: run bookmark test, debug failing test

a632578

test: pylint resolve

41cf857

test: run all data extenstion stream test in bookmark test

ded7b50

test: updated data extension code

d90d1a3

harshpatel4_crest added 2 commits September 8, 2021 11:17

updated the test to run data extension stream

5263ae0

run all tests

a4092ae

karanpanchal-crest approved these changes Sep 8, 2021

View reviewed changes

harshpatel4_crest added 2 commits September 8, 2021 18:25

added sys.exit and updated pylint and astroid to latest versions

093ec08

resolve pylint

972d269

hpatel41 requested review from cosimon, KAllan357 and kspeer825 September 13, 2021 13:02

kspeer825 suggested changes Sep 17, 2021

View reviewed changes

harshpatel4_crest added 2 commits September 17, 2021 18:21

updated the files as per comments

622745f

updated the code

627906b

hpatel41 requested review from kspeer825, luandy64 and zachharris1 September 23, 2021 11:09

KrisPersonal approved these changes Oct 12, 2021

View reviewed changes

hpatel41 and others added 3 commits October 13, 2021 11:44

TDL-14621: Add retry logic to requests and TDL-14622: Retry Connectio…

0418198

…nResetErrors (#71) * added backoff for certain errors * resolve pylint * updated decorator location * added unittests * added comment * added comments

resolved merge conflicts: crest-master branch

c273712

hpatel41 mentioned this pull request Oct 13, 2021

Crest Work to Merge #72

Merged

KrisPersonal mentioned this pull request Oct 14, 2021

Bump to version 1.7.0 #80

Merged

kspeer825 reviewed Nov 15, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TDL-14989: Check best practices #66

TDL-14989: Check best practices #66

hpatel41 commented Aug 24, 2021 •

edited

Loading

dbshah1212 Aug 26, 2021

hpatel41 Aug 27, 2021

dbshah1212 Aug 26, 2021

hpatel41 Aug 27, 2021

dbshah1212 Aug 30, 2021

hpatel41 Sep 24, 2021

dbshah1212 Aug 30, 2021

hpatel41 Sep 24, 2021

dbshah1212 Aug 31, 2021

hpatel41 Sep 24, 2021

dbshah1212 Aug 31, 2021

karanpanchal-crest Sep 8, 2021

hpatel41 Sep 24, 2021

kspeer825 Sep 17, 2021

hpatel41 Sep 17, 2021

kspeer825 Sep 17, 2021

hpatel41 Sep 17, 2021

kspeer825 Sep 17, 2021

kspeer825 Oct 4, 2021

hpatel41 Oct 5, 2021

kspeer825 left a comment

dbshah1212 commented Nov 16, 2021

TDL-14989: Check best practices #66

Are you sure you want to change the base?

TDL-14989: Check best practices #66

Conversation

hpatel41 commented Aug 24, 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

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

Choose a reason for hiding this comment

kspeer825 left a comment

Choose a reason for hiding this comment

dbshah1212 commented Nov 16, 2021

hpatel41 commented Aug 24, 2021 •

edited

Loading