Conversation
zhaohaidao
changed the title
|
@luoyuxia Hi, this pr is ready. PTAL if you have time. |
zhaohaidao
changed the title
There was a problem hiding this comment.
Pull request overview
This PR significantly improves unit test coverage across the Fluss Rust codebase by adding comprehensive test suites for core modules and client functionality. The changes are almost entirely test additions, with only one functional change to implement actual checksum validation.
Changes:
- Implemented checksum validation in
LogRecordBatch::ensure_valid()(previously a TODO placeholder) - Added extensive test utilities including mock connection helpers and test data builders
- Added comprehensive test coverage for scanner, metadata, credentials, cluster, admin, and FFI modules
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/fluss/src/test_utils.rs | Added test utilities including TestCompletedFetch, mock connection builders, and helper functions for creating test data |
| crates/fluss/src/rpc/transport.rs | Added Test variant to Transport enum for test support with proper cfg annotations |
| crates/fluss/src/rpc/server_connection.rs | Added spawn_mock_server helper and insert_connection_for_test method |
| crates/fluss/src/rpc/mod.rs | Exported test-only ApiKey and Transport types |
| crates/fluss/src/record/arrow.rs | Implemented checksum validation in ensure_valid() and added 6 new test cases |
| crates/fluss/src/metadata/table.rs | Added 13 test cases covering schema, table descriptor, and table info functionality |
| crates/fluss/src/cluster/cluster.rs | Added 6 test cases for cluster construction and metadata handling |
| crates/fluss/src/client/table/scanner.rs | Added 30+ test cases for scanner functionality including error handling and fetching logic |
| crates/fluss/src/client/table/remote_log.rs | Added 8 test cases for remote log operations and download futures |
| crates/fluss/src/client/table/log_fetch_buffer.rs | Added 10 test cases for fetch buffer management and error propagation |
| crates/fluss/src/client/mod.rs | Exported test-only types for internal testing |
| crates/fluss/src/client/table/mod.rs | Changed log_fetch_buffer visibility to pub(crate) for test access |
| crates/fluss/src/client/metadata.rs | Added 5 test cases for metadata operations and cluster updates |
| crates/fluss/src/client/credentials.rs | Added 2 test cases for credentials cache and token handling |
| crates/fluss/src/client/connection.rs | Added 3 test cases for connection management and admin creation |
| crates/fluss/src/client/admin.rs | Added 2 comprehensive test cases covering all admin API operations |
| bindings/cpp/src/types.rs | Added 3 test cases for FFI type conversions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@luoyuxia Hi, Conflicts are addressed. PTAL if u have time. |
luoyuxia
left a comment
luoyuxia
left a comment
There was a problem hiding this comment.
@zhaohaidao Thanks for the pr. But I feel like it's a little big for me to reivew. Could you please split this pr into more small pull requests for better review.
| schema: ffi::FfiSchema { | ||
| columns: vec![ffi::FfiColumn { | ||
| name: "bad".to_string(), | ||
| data_type: 999, |
There was a problem hiding this comment.
nit: add comment:
999 is not an valid data type.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn admin_requests_round_trip() -> Result<()> { |
There was a problem hiding this comment.
Could we directly cover it in IT case for any missing method? The current testing looks to me doesn't touch the core code path that need to be test.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn get_or_create_writer_client_is_cached() -> Result<()> { |
There was a problem hiding this comment.
I’m wondering if we really need these tests. Most of them seem to verify very trivial logic (like basic getters and simple caching) that is unlikely to break. I'm concerned they might just increase our maintenance burden without providing much validation value.
|
|
||
| const API_UPDATE_METADATA: i16 = 1012; | ||
|
|
||
| fn build_table_info(table_path: TablePath, table_id: i64) -> TableInfo { |
There was a problem hiding this comment.
can it be replaced with methods in test_utils?
| TableInfo::of(table_path, table_id, 1, table_descriptor, 0, 0) | ||
| } | ||
|
|
||
| fn build_cluster(table_path: &TablePath, table_id: i64) -> Arc<Cluster> { |
2 participants
Purpose
Linked issue: close #xxx
Brief change log
Tests
API and Format
Documentation