Add more tests & fix a data deletion bug in AmazonS3InstallationStore#1081
Add more tests & fix a data deletion bug in AmazonS3InstallationStore#1081seratch merged 2 commits intoslackapi:mainfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1081 +/- ##
==========================================
+ Coverage 83.86% 85.66% +1.79%
==========================================
Files 99 99
Lines 9304 9324 +20
==========================================
+ Hits 7803 7987 +184
+ Misses 1501 1337 -164
Continue to review full report at Codecov.
|
seratch
left a comment
There was a problem hiding this comment.
comments for reviewers
| except Exception as e: # skipcq: PYL-W0703 | ||
| message = f"Failed to find bot installation data for enterprise: {e_id}, team: {t_id}: {e}" | ||
| self.logger.warning(message) | ||
| raise SlackClientConfigurationError(message) |
There was a problem hiding this comment.
For deletion method failures, we should raise an error than just writing warning-level logs
| raise SlackClientConfigurationError(message) | ||
|
|
||
| try: | ||
| no_user_id_key = key.replace(f"-{user_id}", "") |
There was a problem hiding this comment.
When saving a new installation, this implementation stores two historical data:
/111.222/E111-T111/installer-1627859348.984687 and /111.222/E111-T111/installer-U111-1627859348.984687 to support both find_installation(enterprise_id, team_id) and find_installation(enterprise_id, team_id, user_id) without performance overhead (= scanning all the keys under the directory). This deletion method used to delete only the first one.
| if c.get("Key") not in deleted_keys | ||
| ] | ||
| # If only installer-latest remains, we should delete the one as well | ||
| if len(keys) == 1 and keys[0].endswith("installer-latest"): |
There was a problem hiding this comment.
If only the /111.222/E111-T111/installer-latest remains here, this means the installer-latest is no longer valid.
| keys = [ | ||
| c.get("Key") | ||
| for c in objects.get("Contents", []) | ||
| if c.get("Key") not in deleted_keys |
There was a problem hiding this comment.
As S3 storage is now strongly consistent, we may not need to remove these keys, though. https://aws.amazon.com/blogs/aws/amazon-s3-update-strong-read-after-write-consistency/
| import unittest | ||
|
|
||
| import boto3 | ||
| from moto import mock_s3 |
There was a problem hiding this comment.
We have been using moto in bolt-python project already but this is the first test using it in this project https://github.com/spulec/moto#example-on-unittest-usage
|
Let me know by the end of this week if anyone has comments on these changes |
Summary
This pull request adds more unit tests for better code coverage. Particularly, the following patterns are now covered:
While writing the tests, I found a bug in AmazonS3InstallationStore's installation deletion methods. This pull request fixes the bug as well.
Category (place an
xin each of the[ ])/docs-src(Documents, have you run./docs.sh?)/docs-src-v2(Documents, have you run./docs-v2.sh?)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements (place an
xin each[ ])python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.