change error for missing crt. use list of crt checksums instead #2906
Conversation
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## develop #2906 +/- ##
===========================================
- Coverage 93.44% 93.41% -0.04%
===========================================
Files 63 63
Lines 13551 13554 +3
===========================================
- Hits 12663 12661 -2
- Misses 888 893 +5
☔ View full report in Codecov by Sentry. |
| _CRT_CHECKSUM_CLS = { | ||
| "crc32": CrtCrc32Checksum, | ||
| "crc32c": CrtCrc32cChecksum, | ||
| } |
There was a problem hiding this comment.
Saving this to a variable instead lets us validate the keys match the hardcoded list when the CRT is present.
We can't take the list of keys when the CRT is present because it will be empty and is needed for the custom exception.
botocore/httpchecksum.py
Outdated
| if not HAS_CRT and algorithm_name in crt_supported_algorithms: | ||
| raise MissingDependencyException( | ||
| msg=( | ||
| "Using CRC32C requires an additional dependency. You will " |
There was a problem hiding this comment.
| "Using CRC32C requires an additional dependency. You will " | |
| f"Using {algorithm_name} requires an additional dependency. You will " |
There was a problem hiding this comment.
Upper cased here for styling. They're upper cased in the documentation as well so I think it should be like that as well in the error message surfaced to the user.
tests/unit/test_httpchecksum.py
Outdated
| @requires_crt() | ||
| def test_checksum_cls_crt_supported_algorithms_in_sync(self): | ||
| self.assertEqual( | ||
| sorted(_CRT_SUPPORTED_CHECKSUM_ALGORITHMS), | ||
| sorted(list(_CRT_CHECKSUM_CLS.keys())), | ||
| ) | ||
|
|
There was a problem hiding this comment.
I know Jojo has tests checking _CHECKSUM_CLS but ideally we're minimizing access to private variables in our tests. This seems fine as a runtime check as it's minimal impact and will be caught in the CRT test suite.
There was a problem hiding this comment.
Just curious, can you explain the preference of calling the assertion at runtime instead of exposing a private variable in the test suite? Is this just a design principle or is there potential risk to exposing private attributes outside of runtime?
Follow up to #2905. Use
MissingDependencyExceptionand check against extendible list of CRT checksums rather than single strings. Added test to validate CRT checksum list is in sync with checksum class dictionary keys.