Crest Work #63

hpatel41 · 2022-09-27T05:07:43Z

Description of change

Manual QA steps

Risks

Rollback steps

revert this branch

* Added missing assersions in tap-tester tests * updated tap-tester tests * added docstring for test case classes * updated test cases Co-authored-by: harshpatel4crest <[email protected]>

* updated readme file, init and added unittest * updated config.yml file * added logger for sync mode * formatted unitttests

* added unittest of sync.py * updated config.yml file * added unittest for unknown stream sync and updated sync code * updated unittests * formatted unittest

* Added missing fields & add shared schema * Updated types * Updated types * Reverted datatype for some fields * Updated datatype for QtyOnHand Co-authored-by: harshpatel4crest <[email protected]>

* Added minor version in requests * Added unit tests * Removed workaround for some fields as minorversion is added * Fixed all field test * Updated code comment * resolved unittest failure Co-authored-by: harshpatel4crest <[email protected]>

* added exception handling * added parameterized import in config.yml file * updated test case name * handled other 4XX error and updated test cases * added test case * updated test case * added pylint * updated formatting * formatted test case

* added currently syncing functionality * added interrupted sync tap-tester test * updated test case * Updated the tests method to use common test name to use token chaining * Updated the test method to use common test name to use token chaining * updated interrupted sync test * updated interrupted sync test * added docstring for test case name * updated comment indentation * updated tap-tester tests * updated interrupted sync test case * updated interrupted sync test case * formatted test case * resolve unittest failure Co-authored-by: RushT007 <[email protected]>

dsprayberry

Note that I have not reviewed the last 12 files of this PR, but wanted to leave initial feedback so that those requested changes can be made.

tap_quickbooks/__init__.py

tap_quickbooks/sync.py

tap_quickbooks/client.py

tests/test_quickbooks_automatic_fields.py

RushiT0122 · 2022-10-18T04:40:55Z

@sgandhi1311 please fix the review comments.

tests/unittests/test_currently_syncing.py

tests/test_quickbooks_interrupted_sync.py

dsprayberry · 2022-11-03T14:22:45Z

tests/unittests/test_backoff.py

+    @mock.patch('time.sleep')
+    @mock.patch('tap_quickbooks.client.QuickbooksClient._write_config')
+    @mock.patch('requests_oauthlib.OAuth2Session.request')
+    @mock.patch('tap_quickbooks.client.LOGGER.info')


Is this the correct order for the decorators? If so, is there a way to make this more clear? test_backoff_for_OAuth2Session has arguments in an order that doesn't quite match the above given that the parameterized expand would be test_name, test_data, expected_data but then sleep through logger are in reversed order from the function args

Yes, the order is correct.
Using parametrized expand, multiple test cases will be created during execution. Function args are given in the same order as in the @parametrized.expand decorator. Ref - link
For @mock.patch decorators, it will follow the bottom-up approach. So the first function arg will relate to the lowermost patch decorator.

Any suggestions to make it more clear?

I don't have any ideas tbh because a comment feels clunky. I guess I just haven't seen parameterized used with decorators or used that many decorators before 🤷

dsprayberry · 2022-11-03T14:38:03Z

tests/unittests/test_minor_version.py

+@mock.patch('tap_quickbooks.client.OAuth2Session.request')
+@mock.patch('tap_quickbooks.client.QuickbooksClient.get')
+class TestMinorVersion(unittest.TestCase):
+    '''
+    Tests to verify minor_version works as expected.
+    '''
+
+    def test_minor_version(self,mocked_get, mocked_request):


Is this the correct order for the decorators? If so, is there a way to make this more clear? each test function takes arguments in the opposite order of the decorators

Yes, the order is correct.
For @mock.patch decorators, it will follow the bottom-up approach. So the first function arg will relate to the lowermost patch decorator.

Any suggestions to make it more clear?

dsprayberry · 2022-11-03T14:40:14Z

tests/unittests/test_sync.py

+    @mock.patch('tap_quickbooks.client.QuickbooksClient.get')
+    @mock.patch('singer.write_record')
+    @mock.patch('singer.write_state')
+    def test_sync_accounts(self, mock_write_state, mock_write_rcord, mock_get):


Is this the correct order for the decorators? If so, is there a way to make this more clear? each test function takes arguments in the opposite order of the decorators

Please refer to the above response.

dsprayberry · 2022-11-03T14:40:24Z

tests/unittests/test_sync.py

+    @mock.patch('tap_quickbooks.client.QuickbooksClient.get')
+    @mock.patch('singer.write_record')
+    @mock.patch('singer.write_state')
+    @mock.patch('tap_quickbooks.client.QuickbooksClient.__init__', return_value=None)
+    def test_sync_unknown_stream(self, mock_init, mock_write_state, mock_write_rcord, mock_get):


Same comment as above

dsprayberry · 2022-11-03T14:40:32Z

tests/unittests/test_sync.py

+    @mock.patch('tap_quickbooks.client.QuickbooksClient.get')
+    @mock.patch('singer.write_record')
+    @mock.patch('singer.write_state')
+    @mock.patch('tap_quickbooks.streams.ReportStream.parse_report_columns')
+    @mock.patch('tap_quickbooks.streams.ReportStream.parse_report_rows')
+    @mock.patch('tap_quickbooks.streams.ReportStream.day_wise_reports')
+    @mock.patch('singer.utils.now')
+    @mock.patch('tap_quickbooks.client.QuickbooksClient.__init__', return_value=None)
+    def test_sync_report_stream(self, test_name, test_data, mock_init, mock_now, mock_day_wise_report, mock_report_rows, mock_report_columns, mock_write_state, mock_write_record, mock_get):


Same comment as above

dsprayberry · 2022-11-03T14:40:41Z

tests/unittests/test_sync.py

+    @mock.patch('tap_quickbooks.client.QuickbooksClient.get')
+    @mock.patch('singer.write_record')
+    @mock.patch('singer.write_state')
+    @mock.patch('tap_quickbooks.client.QuickbooksClient.__init__', return_value=None)
+    def test_sync_deleted_stream(self, test_name, test_data_count, exp_write_state_count, exp_get_count, mock_init, mock_write_state, mock_write_record, mock_get):


Same comment as above

- removed currently syncing unit test case. - In interrupt sync, made changes according to review comments

RushiT0122 · 2022-11-15T09:04:28Z

tests/test_quickbooks_discovery.py

@@ -40,7 +39,7 @@ def test_run(self):
        self.assertTrue(all([re.fullmatch(r"[a-z_]+", name) for name in found_catalog_names]),
                        msg="One or more streams don't follow standard naming")

-        diff = self.expected_check_streams().symmetric_difference(found_catalog_names)
+        diff = expected_streams.symmetric_difference(found_catalog_names)
        self.assertEqual(len(diff), 0, msg="discovered schemas do not match: {}".format(diff))
        print("discovered schemas are OK")


Please remove this print() statement and replace it with tap-tester logger.

RushiT0122 · 2022-11-15T12:44:33Z

tests/test_quickbooks_all_fields.py

@@ -2,6 +2,7 @@
 from base import TestQuickbooksBase

 class TestQuickbooksAllFields(TestQuickbooksBase):
+    """Test case to verify we are replicating all fields data from the Tap"""

    # remove fields that are replicated when you have account for that specific reqion


typo: reqion

RushiT0122 · 2022-11-15T13:12:18Z

tests/test_quickbooks_automatic_fields.py

-        """
-        return self.expected_check_streams()
-
-
    def test_run(self):


Add the docstring of expectations here

RushiT0122 · 2022-11-15T13:32:25Z

tests/test_quickbooks_automatic_fields.py

                # Verify the number or records exceeds the max_results (api limit)
                pagination_threshold = int(self.get_properties().get(page_size_key))
                self.assertGreater(record_count, pagination_threshold,
-                                   msg="Record count not large enough to gaurantee pagination.")
+                                   msg="Record count not large enough to guarantee pagination.")


This assertion deviates from the test objectives and should be covered in the pagination test, please remove it.

RushiT0122

Add docstrings in the tests to define the objectives of the test (wherever missing).

RushiT0122 · 2022-11-15T13:48:19Z

tests/test_quickbooks_discovery.py

+
+        # Verify there is no duplicate metadata entries
+        self.assertEqual(len(actual_fields), len(set(actual_fields)), msg = "duplicates in the metadata entries retrieved")


Break the long line to keep consistency with above code.

RushiT0122

Requested changes in-line.

RushiT0122

test_quickbooks_interrupted_sync.py: refer test tap-yotpo interrupted sync test

test_quickbooks_sync_all.py: please re-review this test against other tests and see if we really need to test it, otherwise remove this test.
test

RushiT0122 · 2022-11-18T11:02:32Z

tests/test_quickbooks_pagination.py

@@ -29,6 +25,7 @@ def get_properties(self):


    def test_run(self):
+        """Executing run_test with different page_size values for different streams"""


Mention the expectations in the docstring.

…ionary

RushiT0122 · 2022-11-21T15:20:44Z

I noticed init.py in integration tests, if not required we can remove that as well.

sgandhi1311 · 2022-11-21T15:22:35Z

I noticed init.py in integration tests, if not required we can remove that as well.

removed

savan-chovatiya and others added 8 commits September 27, 2022 10:37

TDL-20058: Add missing integration tests. (#61)

b263d30

* Added missing assersions in tap-tester tests * updated tap-tester tests * added docstring for test case classes * updated test cases Co-authored-by: harshpatel4crest <[email protected]>

TDL-13906: Update README.md (#60)

45f1f12

* updated readme file, init and added unittest * updated config.yml file * added logger for sync mode * formatted unitttests

TDL-20063: Add unit test cases (#59)

e7ae21a

* added unittest of sync.py * updated config.yml file * added unittest for unknown stream sync and updated sync code * updated unittests * formatted unittest

TDL-20061: Added missing fields (#58)

3649911

* Added missing fields & add shared schema * Updated types * Updated types * Reverted datatype for some fields * Updated datatype for QtyOnHand Co-authored-by: harshpatel4crest <[email protected]>

added new stream CustomerType (#62)

2c4ebab

RushiT0122 requested review from RushiT0122, bryantgray and dsprayberry October 10, 2022 14:04

dsprayberry requested changes Oct 17, 2022

View reviewed changes

RushiT0122 assigned sgandhi1311 Oct 18, 2022

Fixing the review comments

f5cd937

sgandhi1311 requested a review from dsprayberry October 26, 2022 07:41

dsprayberry requested changes Nov 3, 2022

View reviewed changes

changes -

44eff13

- removed currently syncing unit test case. - In interrupt sync, made changes according to review comments

dsprayberry approved these changes Nov 14, 2022

View reviewed changes

RushiT0122 reviewed Nov 15, 2022

View reviewed changes

RushiT0122 requested changes Nov 15, 2022

View reviewed changes

sgandhi1311 added 4 commits November 16, 2022 09:33

replaced print with logger

05963c6

added docstring to the tests

66c1f29

removed pagination assertion from automatic_fields test

a49e44f

improve readability

9d607dc

sgandhi1311 requested a review from RushiT0122 November 16, 2022 11:05

RushiT0122 requested changes Nov 18, 2022

View reviewed changes

sgandhi1311 added 3 commits November 21, 2022 12:47

fix the doc string formatting

593a650

generate state file dynamically instead of having a static state dict…

cbc370a

…ionary

remove duplicate test cases

b33ad19

removed init.py from tests

892dfa3

RushiT0122 approved these changes Nov 21, 2022

View reviewed changes

Update tap version to 2.0.0 and update the changelog

9580127

kethan1122 approved these changes Nov 30, 2022

View reviewed changes

rdeshmukh15 approved these changes Nov 30, 2022

View reviewed changes

sgandhi1311 merged commit ee37570 into master Nov 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crest Work #63

Crest Work #63

hpatel41 commented Sep 27, 2022 •

edited

Loading

dsprayberry left a comment

RushiT0122 commented Oct 18, 2022

dsprayberry Nov 3, 2022

sgandhi1311 Nov 9, 2022 •

edited

Loading

dsprayberry Nov 14, 2022

dsprayberry Nov 3, 2022

sgandhi1311 Nov 9, 2022

dsprayberry Nov 3, 2022

sgandhi1311 Nov 9, 2022

dsprayberry Nov 3, 2022

dsprayberry Nov 3, 2022

dsprayberry Nov 3, 2022

RushiT0122 Nov 15, 2022

sgandhi1311 Nov 16, 2022

RushiT0122 Nov 15, 2022

sgandhi1311 Nov 16, 2022

RushiT0122 Nov 15, 2022

sgandhi1311 Nov 16, 2022

RushiT0122 Nov 15, 2022

sgandhi1311 Nov 16, 2022

RushiT0122 left a comment

RushiT0122 Nov 15, 2022

sgandhi1311 Nov 16, 2022

RushiT0122 left a comment

RushiT0122 left a comment

RushiT0122 Nov 18, 2022

sgandhi1311 Nov 21, 2022

RushiT0122 commented Nov 21, 2022

sgandhi1311 commented Nov 21, 2022


		# Verify there is no duplicate metadata entries
		self.assertEqual(len(actual_fields), len(set(actual_fields)), msg = "duplicates in the metadata entries retrieved")

		@@ -29,6 +25,7 @@ def get_properties(self):


		def test_run(self):
		"""Executing run_test with different page_size values for different streams"""

Crest Work #63

Crest Work #63

Conversation

hpatel41 commented Sep 27, 2022 • edited Loading

Description of change

Manual QA steps

Risks

Rollback steps

dsprayberry left a comment

Choose a reason for hiding this comment

RushiT0122 commented Oct 18, 2022

Choose a reason for hiding this comment

sgandhi1311 Nov 9, 2022 • 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

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

RushiT0122 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RushiT0122 left a comment

Choose a reason for hiding this comment

RushiT0122 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RushiT0122 commented Nov 21, 2022

sgandhi1311 commented Nov 21, 2022

hpatel41 commented Sep 27, 2022 •

edited

Loading

sgandhi1311 Nov 9, 2022 •

edited

Loading