-
Notifications
You must be signed in to change notification settings - Fork 450
fix(shared/functions): fix slice_dictionary
implementation
#7246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(shared/functions): fix slice_dictionary
implementation
#7246
Conversation
Thanks a lot for your first contribution! Please check out our contributing guidelines and don't hesitate to ask whatever you need. |
Hey @tybug ! Thank you so much for opening this PR, we’ll review it and get back to you. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7246 +/- ##
========================================
Coverage 96.35% 96.35%
========================================
Files 275 275
Lines 12980 12980
Branches 965 965
========================================
Hits 12507 12507
Misses 366 366
Partials 107 107 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hey @tybug this code looks good to me. But I'm curious about the hypothesis you created. I have plans to introduce hypothesis framework in this project to exactly cover cases like this. Do you have a minimal repository where I can those tests? |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for working on this @Liam-DeVoe! APPROVED
Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience! |
As a Hypothesis maintainer, that makes me very happy to hear :) Here's the full bug report written autonomously by the agent, which has the property-based test it wrote inside it: This test is...fine, but it's not particularly idiomatic. The way I might write it instead is: from hypothesis import given, strategies as st
from aws_lambda_powertools.shared.functions import slice_dictionary
@given(
st.dictionaries(st.text(), st.integers()),
st.integers(min_value=1)
)
def test(data, chunk_size):
if not data:
chunks = list(slice_dictionary(data, chunk_size))
assert chunks == []
return
chunks = list(slice_dictionary(data, chunk_size))
reconstructed = {}
for chunk in chunks:
reconstructed.update(chunk)
assert reconstructed == data and possibly the empty-list case could be removed/folded into the standard property as well (I would have to think about it a bit more). |
Issue number: #7252
The implementation of
slice_dictionary
has a bug: it will keep repeating the first chunk, because the stop indexchunk_size
is not incremented as it iterates.fwiw: I found this bug during a research project I'm working on which uses LLMs to write property-based tests in Hypothesis. A property which found this bug was proposed and written "autonomously" by the agent. I wrote this PR myself and take full responsibility for it.
(By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.)