Skip to content

Conversation

jab
Copy link
Member

@jab jab commented Nov 27, 2020

Log results of cache operations for SimpleCache and FileSystemCache as a first step toward improving observability (#182, #17). The remaining cache backends can be updated similarly in followup work as desired.

Also include some minor code improvements (see my review comments).

Run pytest with --log-cli-level=debug for a quick way to view example log output from these changes.

UPDATE: All tests passed on Travis. ✔️

Log results of cache operations for SimpleCache and FileSystemCache as
a start at improving observability (pallets-eco#182, pallets-eco#17).

Also include some minor code improvements.
hit_or_miss = "hit"
try:
result = pickle.loads(value)
except Exception as exc:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quoting https://docs.python.org/3/library/pickle.html#pickle.UnpicklingError

Note that other exceptions may also be raised during unpickling, including (but not necessarily limited to) AttributeError, EOFError, ImportError, and IndexError.

so I broadened the except to catch Exception, but narrowed it around just the pickle.loads() call.

@@ -31,6 +38,7 @@ def test_cache_delete_many(app, cache):
assert cache.get("hi") is not None


@pytest.mark.skipif(HAS_NOT_REDIS, reason="requires Redis")
def test_cache_unlink(app, redis_server):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked like this test was missing the skipif(HAS_NOT_REDIS) that the other Redis-requiring tests have, so I fixed that while I was in here.

@jab jab mentioned this pull request Nov 27, 2020
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 78.885% when pulling 84c5d85 on jab:log into 9e1b157 on sh4nks:master.

@sh4nks sh4nks merged commit a3e7160 into pallets-eco:master Nov 30, 2020
@sh4nks
Copy link
Collaborator

sh4nks commented Nov 30, 2020

Thanks a lot!

@jab
Copy link
Member Author

jab commented Nov 30, 2020

My pleasure, and thank you for merging!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants