-
Notifications
You must be signed in to change notification settings - Fork 2k
[TST][rust-log-service]: add database_name parameter to log service test helpers #6126
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: rescrv/rls3
Are you sure you want to change the base?
Conversation
…est helpers Refactor test helper functions to accept a db_name parameter instead of using hardcoded "test_db" strings: - push_log_to_server - validate_log_on_server - get_enum_offset_on_server - update_compact_offset_on_server - validate_dirty_log_on_server - garbage_collect_unused_logs - test_push_pull_logs, test_dirty_logs, test_fork_logs, etc. Add new k8s mcmr integration test variants that use "topo#dbname" as the database name pattern to verify the log service works correctly with topology-prefixed database names used in multi-cluster deployments. NB: There is no switching of behavior per topo/mcmr; this is scaffold. Co-authored-by: AI
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
These adjustments ensure every helper call site now passes the intended database identifier, and the new MCMR variants run alongside the existing database-name scenarios to confirm compatibility with topology-prefixed naming. Affected Areas• This summary was automatically generated by @propel-code-bot |
| let initial_request = UpdateCollectionLogOffsetRequest { | ||
| collection_id: collection_id_str.clone(), | ||
| log_offset: 100, | ||
| database_name: "test_db".to_string(), |
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.
[Logic] This test variant is intended to verify behavior with topology-prefixed database names ("topo#dbname"), but it currently uses the hardcoded "test_db" string. Please update all 5 occurrences in this function to use "topo#dbname".
Additionally, since this test significantly duplicates the logic of test_k8s_integration_update_collection_log_offset_never_moves_backwards, consider extracting the common logic into a shared helper function that accepts db_name as a parameter.
Context for Agents
This test variant is intended to verify behavior with topology-prefixed database names (`"topo#dbname"`), but it currently uses the hardcoded `"test_db"` string. Please update all 5 occurrences in this function to use `"topo#dbname"`.
Additionally, since this test significantly duplicates the logic of `test_k8s_integration_update_collection_log_offset_never_moves_backwards`, consider extracting the common logic into a shared helper function that accepts `db_name` as a parameter.
File: rust/log-service/src/lib.rs
Line: 4718|
Found 1 test failure on Blacksmith runners: Failure
|
Description of changes
Refactor test helper functions to accept a db_name parameter instead of
using hardcoded "test_db" strings:
Add new k8s mcmr integration test variants that use "topo#dbname" as the
database name pattern to verify the log service works correctly with
topology-prefixed database names used in multi-cluster deployments.
NB: There is no switching of behavior per topo/mcmr; this is scaffold.
Test plan
CI
Migration plan
N/A
Observability plan
N/A
Documentation Changes
N/A
Co-authored-by: AI