implemented backoff for ConnectionResetError and added unittest #169

Rohan397 · 2023-06-12T09:10:00Z

Description of change

https://jira.talendforge.org/browse/TDL-23130

QA steps

automated tests passing
manual qa steps passing (list below)

Risks

Rollback steps

revert this branch

…g test case

RushiT0122 · 2023-06-12T09:27:46Z

tap_shopify/streams/base.py

                          http.client.IncompleteRead,
                          max_tries=MAX_RETRIES,
                          factor=2)
+    @backoff.on_exception(backoff.expo, # ConnectionResetError raised
+                          ConnectionResetError,
+                          max_tries=MAX_RETRIES,
+                          factor=2)


Suggested change

http.client.IncompleteRead,

max_tries=MAX_RETRIES,

factor=2)

@backoff.on_exception(backoff.expo, # ConnectionResetError raised

ConnectionResetError,

max_tries=MAX_RETRIES,

factor=2)

(http.client.IncompleteRead, ConnectionResetError),

max_tries=MAX_RETRIES,

factor=2)

RushiT0122 · 2023-06-12T09:31:09Z

tests/unittests/test_error_handling.py

+    @mock.patch("time.sleep")
+    @mock.patch("pyactiveresource.activeresource.ActiveResource.find")
+    def test_check_access_handle_connection_reset_error(self, mocked_find, mocked_sleep):
+        '''
+        Test retry handling of ConnectionResetError
+        '''
+        # mock 'find' and raise IncompleteRead error
+        mocked_find.side_effect = ConnectionResetError
+        # initialize class
+        locations = Transactions()
+        try:
+            # function call
+            locations.replication_object.find()
+        except ConnectionResetError:
+            pass

-        self.assertEqual(stream.replication_object.find.call_count, 5)
+        # verify we backoff 5 times
+        self.assertEquals(mocked_find.call_count, 5)


[Optional] Here if possible, can we use parameterised unittests?

RushiT0122 · 2023-06-12T09:36:30Z

tests/unittests/test_error_handling.py

+        try:
+            # function call
+            locations.replication_object.find()
+        except ConnectionResetError:
+            pass


Use assertRaises() instead of try-catch.

RushiT0122

Suggestions in-line.

kethan1122 · 2023-06-12T09:41:10Z

tests/unittests/test_error_handling.py


-        self.assertEqual(stream.replication_object.find.call_count, 5)
+        # verify we backoff 5 times
+        self.assertEquals(mocked_find.call_count, 5)


Instead of hard coding the call_count to be 5, we can import MAX_RETRIES from from tap_shopify.streams.base and use it here for assertion

I think, we should not use MAX_RETRIES from tap_shopify.streams.base in assertion because unittests would not catch any deviation in MAX_RETRIESfrom the expectations.

tap_shopify/streams/base.py

tests/unittests/test_error_handling.py

…io/tap-shopify into TDL-23130/improve-error-handling

Co-authored-by: Rushikesh Todkar <98420315+RushiT0122@users.noreply.github.com>

…io/tap-shopify into TDL-23130/improve-error-handling

RushiT0122

Please confirm if make file is not required elsewhere before merging the code. Otherwise rest looks good to me, so approved!

luandy64 · 2023-06-13T14:37:21Z

tests/unittests/test_error_handling.py

Problem 1: Don't use exec

Parameterizing unit tests is fine, in theory. However I think exec is a tool we should only reach for when we have no other options.

This is not one of those times. In Python, functions are objects too. And objects can be passed around as parameters to functions.

Here's some test code that runs and fails, but illustrates my point:

import unittest from unittest import mock from urllib.error import URLError from tap_shopify.streams.transactions import Transactions from tap_shopify.streams.orders import Orders from parameterized import parameterized class TestShopifyConnectionResetErrorHandling(unittest.TestCase): @parameterized.expand([ (URLError('<urlopen error [Errno 104] Connection reset by peer>'), Orders.replication_object.find, 5), (ConnectionResetError(''), Transactions.replication_object.find, 5), ]) @mock.patch("time.sleep") @mock.patch("pyactiveresource.activeresource.ActiveResource.find") def test_check_access(self, error, func, expected_retries, mocked_find, mocked_sleep): ''' Test retry handling of URLError and ConnectionResetError ''' mocked_find.side_effect = error with self.assertRaises(type(error)): func() # verify we backoff expected number of times self.assertEqual(mocked_find.call_count, expected_retries)

And the test output

$ nosetests tests/unittests/test_error_handling.py FINFO Backing off find(...) for 1.8s (ConnectionResetError) INFO Backing off find(...) for 0.9s (ConnectionResetError) INFO Backing off find(...) for 2.9s (ConnectionResetError) INFO Backing off find(...) for 6.6s (ConnectionResetError) ERROR Giving up find(...) after 5 tries (ConnectionResetError) . ====================================================================== FAIL: Test retry handling of URLError and ConnectionResetError [with *args=(URLError('<urlopen error [Errno...ify.resources.order.Order'>>, 5)] ---------------------------------------------------------------------- Traceback (most recent call last): File "/usr/local/share/virtualenvs/tap-shopify/lib/python3.9/site-packages/parameterized/parameterized.py", line 620, in standalone_func return func(*(a + p.args), **p.kwargs, **kw) File "/home/ubuntu/.pyenv/versions/3.9.6/lib/python3.9/unittest/mock.py", line 1337, in patched return func(*newargs, **newkeywargs) File "/usr/local/share/virtualenvs/tap-shopify/lib/python3.9/site-packages/parameterized/parameterized.py", line 93, in dummy_func return orgfunc(*args, **kwargs) File "/home/ubuntu/.pyenv/versions/3.9.6/lib/python3.9/unittest/mock.py", line 1337, in patched return func(*newargs, **newkeywargs) File "/opt/code/tap-shopify/tests/unittests/test_error_handling.py", line 28, in test_check_access self.assertEqual(mocked_find.call_count, expected_retries) AssertionError: 1 != 5 ---------------------------------------------------------------------- Ran 2 tests in 0.005s

This fails because in my example I changed Orders to call Orders.replication_object.find instead of Orders.call_api like in your test.

I encourage you to try to parameterize this test yourself and get it passing for Orders.call_api and Transactions.replication_object.find. And DM me after and we can talk about it.

But when you get that passing, I would still ask for changes to the test because

Problem 2: One unit test should test one function

I understand the desire to find a way to dry up the code remove as much boilerplate code as possible because this file is only concerned with shopify_error_handling and whether it works for both Orders and Transactions.

Parameterizing the test like this though sacrifices readability of the test for what? For a 45% reduction in lines of code?

That's not a trade worth making in my opinion. So this one class TestShopifyConnectionResetErrorHandling should just have like a test_orders_retries_on_URLError and a test_transactions_retries_on_ConnectionResetError.

Also, in this particular instance, the abstraction needed to parameterize the test isn't perfect. call_api is a method and needs the orders object to actually be initialized. But find doesn't need transactions because find is on the ShopifySDK.Transaction object, which is already initialized and attached to the tap_shopify.Transaction object (or find is a static method, which highlights my point even more).

So going back to this code

import unittest from unittest import mock from urllib.error import URLError from tap_shopify.streams.transactions import Transactions from tap_shopify.streams.orders import Orders from parameterized import parameterized class TestShopifyConnectionResetErrorHandling(unittest.TestCase): @parameterized.expand([ (URLError('<urlopen error [Errno 104] Connection reset by peer>'), Orders.call_api, 5), (ConnectionResetError(''), Transactions.replication_object.find, 5), ]) @mock.patch("time.sleep") @mock.patch("pyactiveresource.activeresource.ActiveResource.find") def test_check_access(self, error, func, expected_retries, mocked_find, mocked_sleep): ''' Test retry handling of URLError and ConnectionResetError ''' mocked_find.side_effect = error with self.assertRaises(type(error)): func() # verify we backoff expected number of times self.assertEqual(mocked_find.call_count, expected_retries)

And running it

$ nosetests tests/unittests/test_error_handling.py EINFO Backing off find(...) for 0.8s (ConnectionResetError) INFO Backing off find(...) for 0.1s (ConnectionResetError) INFO Backing off find(...) for 7.8s (ConnectionResetError) INFO Backing off find(...) for 7.6s (ConnectionResetError) ERROR Giving up find(...) after 5 tries (ConnectionResetError) . ====================================================================== ERROR: Test retry handling of URLError and ConnectionResetError [with *args=(URLError('<urlopen error [Errno....call_api at 0x7f610b4533a0>, 5)] ---------------------------------------------------------------------- Traceback (most recent call last): File "/usr/local/share/virtualenvs/tap-shopify/lib/python3.9/site-packages/parameterized/parameterized.py", line 620, in standalone_func return func(*(a + p.args), **p.kwargs, **kw) File "/home/ubuntu/.pyenv/versions/3.9.6/lib/python3.9/unittest/mock.py", line 1337, in patched return func(*newargs, **newkeywargs) File "/usr/local/share/virtualenvs/tap-shopify/lib/python3.9/site-packages/parameterized/parameterized.py", line 93, in dummy_func return orgfunc(*args, **kwargs) File "/home/ubuntu/.pyenv/versions/3.9.6/lib/python3.9/unittest/mock.py", line 1337, in patched return func(*newargs, **newkeywargs) File "/opt/code/tap-shopify/tests/unittests/test_error_handling.py", line 23, in test_check_access func() File "/usr/local/share/virtualenvs/tap-shopify/lib/python3.9/site-packages/backoff/_sync.py", line 94, in retry ret = target(*args, **kwargs) File "/usr/local/share/virtualenvs/tap-shopify/lib/python3.9/site-packages/backoff/_sync.py", line 94, in retry ret = target(*args, **kwargs) File "/usr/local/share/virtualenvs/tap-shopify/lib/python3.9/site-packages/backoff/_sync.py", line 94, in retry ret = target(*args, **kwargs) [Previous line repeated 1 more time] File "/opt/code/tap-shopify/tap_shopify/streams/base.py", line 149, in wrapper return fnc(*args, **kwargs) TypeError: call_api() missing 2 required positional arguments: 'self' and 'query_params' ---------------------------------------------------------------------- Ran 2 tests in 0.005s FAILED (errors=1)

You can see that Transactions passes without your initialize the object code stream = Context.stream_objects[stream_name]().

So forcing two things to be similar when they're not is another code smell to me

luandy64

The PR looks good. Just delete that extra code and I will approve

luandy64 · 2023-06-14T13:36:48Z

tests/unittests/test_error_handling.py

+        mocked_find.side_effect = ConnectionResetError
+
+        # initialize class
+        locations = Transactions()


Any particular reason this is named locations?

tests/unittests/test_error_handling.py

implemented backoff for ConnectionResetError and added a correspondin…

2385d5c

…g test case

RushiT0122 self-requested a review June 12, 2023 09:24

RushiT0122 reviewed Jun 12, 2023

View reviewed changes

RushiT0122 requested changes Jun 12, 2023

View reviewed changes

kethan1122 reviewed Jun 12, 2023

View reviewed changes

updated commit based on review feedback

cfb9d8e

Rohan397 commented Jun 13, 2023

View reviewed changes

tap_shopify/streams/base.py Outdated Show resolved Hide resolved

Update tap_shopify/streams/base.py

2c31283

RushiT0122 reviewed Jun 13, 2023

View reviewed changes

tests/unittests/test_error_handling.py Outdated Show resolved Hide resolved

Rohan Hejmadi and others added 9 commits June 13, 2023 04:23

add parameterized dependancy for unittests

4668d6e

Merge branch 'TDL-23130/improve-error-handling' of github.com:singer-…

7e683f9

…io/tap-shopify into TDL-23130/improve-error-handling

Update tests/unittests/test_error_handling.py

6e2b0e8

Co-authored-by: Rushikesh Todkar <98420315+RushiT0122@users.noreply.github.com>

remove trailing whitespace

9378e32

Merge branch 'TDL-23130/improve-error-handling' of github.com:singer-…

8b7866e

…io/tap-shopify into TDL-23130/improve-error-handling

Merge branch 'master' into TDL-23130/improve-error-handling

0dc71b1

fixing circleci job

fa4e456

Merge branch 'TDL-23130/improve-error-handling' of github.com:singer-…

2593fbc

…io/tap-shopify into TDL-23130/improve-error-handling

removing makefile since it is redundant

bf222dd

RushiT0122 approved these changes Jun 13, 2023

View reviewed changes

luandy64 requested changes Jun 13, 2023

View reviewed changes

made changes based on review

af769c2

luandy64 reviewed Jun 14, 2023

View reviewed changes

Rohan Hejmadi added 2 commits June 16, 2023 03:59

removed extra code and switched locations to stream

c54237b

remove extra import

d35cc98

luandy64 approved these changes Jun 20, 2023

View reviewed changes

Rohan397 merged commit 539564f into master Jun 20, 2023

Rohan397 mentioned this pull request Jun 20, 2023

bumped version and added to changelog #170

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

implemented backoff for ConnectionResetError and added unittest #169

implemented backoff for ConnectionResetError and added unittest #169

Uh oh!

Rohan397 commented Jun 12, 2023

Uh oh!

RushiT0122 Jun 12, 2023

Uh oh!

RushiT0122 Jun 12, 2023 •

edited

Loading

Uh oh!

RushiT0122 Jun 12, 2023

Uh oh!

RushiT0122 left a comment

Uh oh!

kethan1122 Jun 12, 2023

Uh oh!

RushiT0122 Jun 12, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

RushiT0122 left a comment

Uh oh!

luandy64 Jun 13, 2023

Uh oh!

luandy64 left a comment

Uh oh!

luandy64 Jun 14, 2023

Uh oh!

Uh oh!

Uh oh!

implemented backoff for ConnectionResetError and added unittest #169

implemented backoff for ConnectionResetError and added unittest #169

Uh oh!

Conversation

Rohan397 commented Jun 12, 2023

Description of change

QA steps

Risks

Rollback steps

Uh oh!

RushiT0122 Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

RushiT0122 Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RushiT0122 Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

RushiT0122 left a comment

Choose a reason for hiding this comment

Uh oh!

kethan1122 Jun 12, 2023

Choose a reason for hiding this comment

Uh oh!

RushiT0122 Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RushiT0122 left a comment

Choose a reason for hiding this comment

Uh oh!

luandy64 Jun 13, 2023

Choose a reason for hiding this comment

Problem 1: Don't use exec

Problem 2: One unit test should test one function

Uh oh!

luandy64 left a comment

Choose a reason for hiding this comment

Uh oh!

luandy64 Jun 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RushiT0122 Jun 12, 2023 •

edited

Loading

RushiT0122 Jun 12, 2023 •

edited

Loading

Problem 1: Don't use `exec`