fix: Avoid pagination failures when filtering connection by last without before/after #585
Conversation
…out before/after Fix #576
Reviewer's Guide by SourceryThis pull request addresses issue #576 by enhancing the pagination logic in the apply_window_pagination function to handle cases where the limit is set to sys.maxsize or -1, which are used to mimic 'no limit' scenarios. Additionally, new tests were added to verify the correct behavior of the pagination logic under these conditions. Minor updates were also made to existing tests to improve their robustness. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @bellini666 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 5 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| assert fruit._strawberry_total_count == 10 # type: ignore | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("limit", [-1, sys.maxsize]) |
There was a problem hiding this comment.
suggestion (testing): Add a test case for limit set to zero.
It would be useful to add a test case where the limit is set to zero to verify that the pagination logic correctly handles this scenario.
| @pytest.mark.parametrize("limit", [-1, sys.maxsize]) | |
| @pytest.mark.parametrize("limit", [-1, 0, sys.maxsize]) |
|
|
||
| @pytest.mark.parametrize("limit", [-1, sys.maxsize]) | ||
| @pytest.mark.django_db(transaction=True) | ||
| def test_apply_window_pagination_with_no_limites(limit): |
There was a problem hiding this comment.
nitpick (testing): Typo in test function name.
The function name test_apply_window_pagination_with_no_limites contains a typo. It should be test_apply_window_pagination_with_no_limits.
| for i in range(10): | ||
| models.Fruit.objects.create(name=f"fruit{i}", color=color) |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #585 +/- ##
==========================================
+ Coverage 88.79% 88.82% +0.03%
==========================================
Files 41 41
Lines 3597 3598 +1
==========================================
+ Hits 3194 3196 +2
+ Misses 403 402 -1 ☔ View full report in Codecov by Sentry. |
Fix #576
Summary by Sourcery
This pull request addresses an issue with pagination failures when filtering connections by 'last' without 'before' or 'after' parameters. It includes new test cases to validate the correct behavior of window pagination under various conditions.