Skip to content

[TST] More proptest for rust log service #4771

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

Merged

Conversation

Sicheng-Pan
Copy link
Contributor

@Sicheng-Pan Sicheng-Pan commented Jun 5, 2025

Description of changes

Summarize the changes made by this PR.

  • Improvements & Bug fixes

    • More proptest for rust log service to cover additional methods
      • scout_log
      • update_collection_offset
      • roll_dirty_log
      • cached_get_all_collection_info_to_compact
      • fork_log
    • Add two tests that does the following:
      • Test dirty log under multiple collections: insert a single record to different collections, update the compaction offset in random order. Finally verify that the dirty log has the right collections in each step of the update.
      • Test log fork: insert random data to source collection, fork the log, and add data to both source and fork collections. Finally verify that the logs for both collections are correct.
    • Bug fix
      • Dirty log offset ahead of actual enumeration offset
  • New functionality

    • ...

Test plan

How are these changes tested?

  • Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?

Copy link

github-actions bot commented Jun 5, 2025

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@Sicheng-Pan Sicheng-Pan force-pushed the sicheng/06-05-_tst_more_proptest_for_rust_log_service branch from 4982437 to b072e5f Compare June 6, 2025 01:20
@Sicheng-Pan Sicheng-Pan marked this pull request as ready for review June 6, 2025 18:43
@Sicheng-Pan Sicheng-Pan requested a review from rescrv June 6, 2025 18:43
Copy link
Contributor

Expanded Proptest and Bug Fixes for Rust Log Service Testing

This PR significantly expands property-based testing (proptest) coverage for the Rust log service, introducing new property tests that exercise additional methods including 'scout_log', 'update_collection_offset', 'roll_dirty_log', 'cached_get_all_collection_info_to_compact', and 'fork_log'. Two comprehensive integration-style tests are added: one verifies correct handling of dirty logs with multiple collections and sequence compactions, and another validates the correctness and isolation of the log forking process. Additionally, a bug fix addresses an issue where the dirty log offset could advance ahead of the enumeration offset, improving correctness of the logging logic. Small adjustments are also made to test and helper function organization, including improvements to Debug impls and test utilities.

Key Changes:
• Added new proptests for log service API surface (scout_log, update_collection_offset, roll_dirty_log, cached_get_all_collection_info_to_compact, fork_log)
• Introduced two high-level property-based tests: multi-collection dirty log state validation and fork_log correctness validation
• Fixed a bug where the dirty log offset could be advanced beyond the enumeration offset
• Refactored and expanded test helper functions for improved reusability
• Improved Debug implementation for OperationRecord to support cleaner test outputs

Affected Areas:
• rust/log-service/src/lib.rs (test module, logic related to dirty log/compaction/forking)
• rust/types/src/record.rs (Debug impl for OperationRecord, test display improvements)
• rust/wal3/src/writer.rs (small logic fix for mark_dirty position)

Potential Impact:

Functionality: System functionality remains unaffected in mainline execution paths, but correctness and resilience of log service is improved via broader test coverage and a bug fix.

Performance: No direct impact; changes are limited to test code and a correctness fix in mark_dirty calls.

Security: No security implications.

Scalability: No scalability concerns; changes are focused on correctness and test coverage.

Review Focus:
• Correctness and clarity of new property test logic, especially multi-collection and fork scenarios.
• Check that bug fix in wal3/src/writer.rs addresses possible incorrect mark_dirty positions.
• Alignment of new tests with documented behavior and billing of log service API.
• Review any changes in struct Debug output for possible effect on logging/debuggability.

Testing Needed

• Run cargo test to verify all new and existing tests pass.
• Proptest runs may be lengthy; ensure test infra can handle increased test permutations.
• Focus on edge cases around log forking, compaction offset updates, and multi-collection log management.

Code Quality Assessment

rust/log-service/src/lib.rs: Test organization improved; helper functions and property tests are logically grouped and maintainable.

rust/types/src/record.rs: Debug impl is correct and avoids leaking bulky internal fields; testability improved.

rust/wal3/src/writer.rs: The mark_dirty fix corrects record offset handling when updating dirty state.

Best Practices

Testing:
• Extensive use of property-based testing.
• Isolation of side effects in stateful component tests.
• Helper utilities for DRY test logic.

Bugfix:
• Fix is targeted and covered by test.
• Minimal scope expansion.

Code Organization:
• Logical grouping of test helpers and proptest functions.

Potential Issues

• Expanded use of proptest may lead to increased test execution time or intermittent test failures in CI if shrinking is not handled robustly.
• Property tests are dependent on randomized data; rare edge cases might still exist if certain contracts are not sufficiently expressed in test assertions.
• Interaction between compaction and enumeration offsets should be reviewed in production paths for possible regressions due to test-motivated code changes.

This summary was automatically generated by @propel-code-bot

@Sicheng-Pan Sicheng-Pan merged commit 343bd9a into main Jun 6, 2025
72 checks passed
@Sicheng-Pan Sicheng-Pan deleted the sicheng/06-05-_tst_more_proptest_for_rust_log_service branch June 6, 2025 21:22
Sicheng-Pan pushed a commit that referenced this pull request Jun 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants