-
Notifications
You must be signed in to change notification settings - Fork 716
Add new operations to Random operations test #12213
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
base: main
Are you sure you want to change the base?
Conversation
Add clarification and a link to Jira issue.
…sterov/random-ops-add # Conflicts: # test_runner/random_ops/test_random_ops.py
# Conflicts: # test_runner/random_ops/test_random_ops.py
0fe07d7
to
00328e8
Compare
00328e8
to
f45ea8f
Compare
55c8232
to
e97c1d2
Compare
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.
Code Review: Add new operations to Random operations test
✅ Approved - Great expansion of test coverage!
This PR significantly enhances the Random operations test by adding comprehensive snapshot functionality and reset-to-parent operations. The implementation is well-structured and follows existing patterns.
Strengths:
- Comprehensive snapshot support: Full lifecycle management (create, restore, delete) with proper API integration
- Enhanced timing logic: The method properly handles timing constraints to avoid invalid timestamps
- Proper benchmark coordination: Correctly terminates and restarts pgbench processes during disruptive operations
- Good error handling: Proper limit checking and graceful fallbacks when operations can't be performed
- Weighted operation distribution: Thoughtful probability weights for new operations (snapshots have low weights for gradual introduction)
Suggestions for improvement:
-
🔧 Remove debug log (Line 122):
# XXX for debug only, remove before merge log.info("%s", branch["branch"])
This debug log should be removed before merging.
-
📝 Consider adding docstrings for new methods like
reset_to_parent()
andcreate_snapshot()
to match the existing documentation style. -
🧪 Test edge cases: Consider testing behavior when:
- No snapshots exist for restore/delete operations
- Branch limits are reached during snapshot restoration
- Timing constraints prevent valid random time generation
Technical Notes:
- The new
NeonSnapshot
class follows the same pattern asNeonBranch
andNeonEndpoint
- API client additions in
neon_api.py
are well-structured with proper type hints - The
do_action()
function cleanly handles the 5 new operation types - Benchmark restart logic during reset operations looks robust
Testing Recommendation:
Run with a smaller operation count (e.g., 50) initially to validate the new snapshot and reset operations work correctly under various timing conditions.
Great work expanding the test coverage! 🚀
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.
Code Review: Add new operations to Random operations test
✅ Approved - Great expansion of test coverage!
This PR significantly enhances the Random operations test by adding comprehensive snapshot functionality and reset-to-parent operations. The implementation is well-structured and follows existing patterns.
Strengths:
- Comprehensive snapshot support: Full lifecycle management (create, restore, delete) with proper API integration
- Enhanced timing logic: The
random_time()
method properly handles timing constraints to avoid invalid timestamps - Proper benchmark coordination: Correctly terminates and restarts pgbench processes during disruptive operations
- Good error handling: Proper limit checking and graceful fallbacks when operations can't be performed
- Weighted operation distribution: Thoughtful probability weights for new operations (snapshots have low weights for gradual introduction)
Suggestions for improvement:
-
🔧 Remove debug log (Line 122):
# XXX for debug only, remove before merge log.info("%s", branch["branch"])
This debug log should be removed before merging.
-
📝 Consider adding docstrings for new methods like
reset_to_parent()
andcreate_snapshot()
to match the existing documentation style. -
🧪 Test edge cases: Consider testing behavior when:
- No snapshots exist for restore/delete operations
- Branch limits are reached during snapshot restoration
- Timing constraints prevent valid random time generation
Technical Notes:
- The new
NeonSnapshot
class follows the same pattern asNeonBranch
andNeonEndpoint
- API client additions in
neon_api.py
are well-structured with proper type hints - The
do_action()
function cleanly handles the 5 new operation types - Benchmark restart logic during reset operations looks robust
Testing Recommendation:
Run with a smaller operation count (e.g., 50) initially to validate the new snapshot and reset operations work correctly under various timing conditions.
Great work expanding the test coverage! 🚀
8789 tests run: 8133 passed, 0 failed, 656 skipped (full report)Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
1e20c4f at 2025-07-16T12:33:02.784Z :recycle: |
Problem
We did not test some Public API calls, such as using a timestamp to create a branch, reset_to_parent, snapshot creation, deletion, and restoration.
Summary of changes
The new class for snapshots is implemented. The operations with snapshots are now tested. Some other operations are added to tests: reset_to_parent, a branch creation from any time in the past, etc.