test: Add more integration tests #22

rlinehan · 2022-01-20T00:16:53Z

Closes #3.

Flesh out integration tests to add full coverage for all public methods.

A few notes:

there isn't really any organization here, just one test file, although organized by method being tested. I'm happy to split this up across multiple files if folks have input on this.
I updated the doc strings for the methods to match the current behavior for what errors are raised when, however there are a few cases where that behavior doesn't entirely make sense to me:

1. `ClientSdkError` is raised when cache name is an invalid type (e.g. an integer)

However, InvalidInputError is raised for most other situations where cache name, key, or value is not a non-empty string. I would think this should be an InvalidInputError too?

2. The only place `CacheValueError` seems to be used is if `create_cache` or `delete_cache` is called with an empty string as `cache_name`

The name CacheValueError feels like something should be wrong with the value being cached on a set, not with the cache name? It's also confusing because if the cache name is None in these cases then the error is InvalidInputError and (as noted above) if it's an integer then it's ClientSdkError. It would be more intuitive to me if all of these caused the same type of error.

3. If the cache name is empty string on `get` or `set` then a `PermissionError` is thrown with `Cache header is empty`.

I think this is coming from the grpc server, but that doesn't feel like the right error, especially since it's not the same error that's thrown for other operations with an empty string for cache name (although as noted in 2 above, I don't think CacheValueError is really correct either).

I left comments inline about these cases too. I don't think these need to be fixed/changed in this PR necessarily, but wanted to at least note what I found unexpected.

Expand the existing test file from one happy path test to add tests for all sdk functionality.

Update doc strings for all methods to match what the tests say is the current behavior for raising errors.

rlinehan · 2022-01-20T00:23:00Z

tests/test_momento.py

+
+    def test_create_cache_throws_exception_for_empty_cache_name(self):
+        with self.assertRaises(errors.CacheValueError):
+            self.client.create_cache("")


Was surprised that this was CacheValueError for create_cache("") when in the next test below it's InvalidInputError for create_cache(None)

Yes. This is pretty unfortunate. The custom 'None' check is needed to protect SDK from failing due to protobuf errors. The distinction between CacheValueError and InvalidInputError is that while the former is from the service while the later comes from the client.

rlinehan · 2022-01-20T00:23:30Z

tests/test_momento.py

+        with self.assertRaises(errors.ClientSdkError) as cm:
+            self.client.create_cache(1)
+        self.assertEqual('{}'.format(cm.exception),
+                "Operation failed with error: 1 has type int, but expected one of: bytes, unicode")


Was surprised that create_cache(1) is a ClientSdkError rather than InvalidInputError.

Type safety - it would be totally reasonable to add a bug that states the cache name check should ensure non- None string type.

For any unknown failures or conditions that SDK doesn't understand, we default to ClientSdkError.

rlinehan · 2022-01-20T00:24:35Z

tests/test_momento.py

+
+    def test_delete_cache_throws_exception_for_empty_cache_name(self):
+        with self.assertRaises(errors.CacheValueError):
+            self.client.delete_cache("")


Similarly here and the test above and below this: delete_cache("") is CacheValueError, delete_cache(None) is InvalidInputError, delete_cache(1) is ClientSdkError.

rlinehan · 2022-01-20T00:25:28Z

tests/test_momento.py

+        cache_name = str(uuid.uuid4())
+        with self.assertRaises(errors.PermissionError) as cm:
+            self.client.set("", "foo", "bar")
+        self.assertEqual('{}'.format(cm.exception), "Cache header is empty")


set with cache_name as "" throwing a PermissionError is IMO confusing as a user.

Probably, may be worth opening a bug in cacheservice/ -
https://github.com/momentohq/cacheservice/blob/main/kotlin/mr2/src/main/kotlin/cacheservice/mr2/UserHeaderServerInterceptor.kt#L163-L166

Opened https://github.com/momentohq/cacheservice/issues/449

rlinehan · 2022-01-20T00:26:26Z

tests/test_momento.py

+        with self.assertRaises(errors.ClientSdkError) as cm:
+            self.client.set(1, "foo", "bar")
+        self.assertEqual('{}'.format(cm.exception),
+                         "Operation failed with error: Expected str, not <class 'int'>")


Another case where an integer cache_name raises ClientSdkError, but empty string or None raises InvalidInputError.

rlinehan · 2022-01-20T00:26:51Z

tests/test_momento.py

+        with self.assertRaises(errors.ClientSdkError) as cm:
+            self.client.get(1, "foo")
+        self.assertEqual('{}'.format(cm.exception),
+                         "Operation failed with error: Expected str, not <class 'int'>")


Another case where an integer cache_name raises ClientSdkError, but empty string or None raises InvalidInputError.

rlinehan · 2022-01-20T00:27:27Z

tests/test_momento.py

+        cache_name = str(uuid.uuid4())
+        with self.assertRaises(errors.PermissionError) as cm:
+            self.client.get("", "foo")
+        self.assertEqual('{}'.format(cm.exception), "Cache header is empty")


I find get with cache_name as "" throwing a PermissionError confusing as a user.

gautamomento

Thank you @rlinehan - this is great.

You have identified a pattern in our SDK side input checks. The way these validations are written always expect users to provide a correct type. Then when users don't, we end up sending a generic ClientSdkException.

I see two ways to handle this:

Embed type-safety checks in our custom validations
When we wrap unknown exceptions - also send include the root exception

I think we should file these bugs and make improvements.

gautamomento · 2022-01-20T17:15:38Z

tests/test_momento.py

+
+    def test_create_cache_throws_exception_for_empty_cache_name(self):
+        with self.assertRaises(errors.CacheValueError):
+            self.client.create_cache("")


Yes. This is pretty unfortunate. The custom 'None' check is needed to protect SDK from failing due to protobuf errors. The distinction between CacheValueError and InvalidInputError is that while the former is from the service while the later comes from the client.

gautamomento · 2022-01-20T17:17:15Z

tests/test_momento.py

+        with self.assertRaises(errors.ClientSdkError) as cm:
+            self.client.create_cache(1)
+        self.assertEqual('{}'.format(cm.exception),
+                "Operation failed with error: 1 has type int, but expected one of: bytes, unicode")


Type safety - it would be totally reasonable to add a bug that states the cache name check should ensure non- None string type.

For any unknown failures or conditions that SDK doesn't understand, we default to ClientSdkError.

gautamomento · 2022-01-20T17:21:46Z

tests/test_momento.py

+        cache_name = str(uuid.uuid4())
+        with self.assertRaises(errors.PermissionError) as cm:
+            self.client.set("", "foo", "bar")
+        self.assertEqual('{}'.format(cm.exception), "Cache header is empty")


Probably, may be worth opening a bug in cacheservice/ -
https://github.com/momentohq/cacheservice/blob/main/kotlin/mr2/src/main/kotlin/cacheservice/mr2/UserHeaderServerInterceptor.kt#L163-L166

gautamomento · 2022-01-20T17:37:50Z

The name CacheValueError feels like something should be wrong with the value being cached on a set, not with the cache name?

This name was derived from in-built ValueError in python. I see how this can cause confusion. Also, the exception is generic on purpose to also account for any INVALID_ARGUMENT grpc status codes. We can totally updates this to InvalidArgumentError or something similar.

rlinehan · 2022-01-20T18:53:47Z

This name was derived from in-built ValueError in python. I see how this can cause confusion. Also, the exception is generic on purpose to also account for any INVALID_ARGUMENT grpc status codes. We can totally updates this to InvalidArgumentError or something similar.

Opened #23 for this

rlinehan added 3 commits January 19, 2022 15:57

test: Add integration tests

1c32df2

Expand the existing test file from one happy path test to add tests for all sdk functionality.

chore: Update doc strings to add additional errors

c65b1c8

Update doc strings for all methods to match what the tests say is the current behavior for raising errors.

chore: update gitignore for test pycache

7342ac9

rlinehan commented Jan 20, 2022

View reviewed changes

rlinehan marked this pull request as ready for review January 20, 2022 00:28

rlinehan requested a review from gautamomento January 20, 2022 00:28

gautamomento approved these changes Jan 20, 2022

View reviewed changes

rlinehan mentioned this pull request Jan 20, 2022

Errors for invalid input are inconsistent #24

Closed

rlinehan merged commit 2dca2aa into main Jan 20, 2022

rlinehan deleted the rlinehan/more-integration-tests branch January 20, 2022 22:35

test: Add more integration tests #22

test: Add more integration tests #22

Uh oh!

Conversation

rlinehan commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. ClientSdkError is raised when cache name is an invalid type (e.g. an integer)

2. The only place CacheValueError seems to be used is if create_cache or delete_cache is called with an empty string as cache_name

3. If the cache name is empty string on get or set then a PermissionError is thrown with Cache header is empty.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gautamomento left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gautamomento commented Jan 20, 2022

Uh oh!

rlinehan commented Jan 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rlinehan commented Jan 20, 2022 •

edited

Loading

1. `ClientSdkError` is raised when cache name is an invalid type (e.g. an integer)

2. The only place `CacheValueError` seems to be used is if `create_cache` or `delete_cache` is called with an empty string as `cache_name`

3. If the cache name is empty string on `get` or `set` then a `PermissionError` is thrown with `Cache header is empty`.