Restructure modules & move some session tests to integration #375
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wprzytula
requested review from
Lorak-mmk and
Copilot
and removed request for
Lorak-mmk
There was a problem hiding this comment.
Pull Request Overview
This PR performs a module restructure and moves session tests to integration tests to improve organization and code maintainability.
- Restructures modules by moving several modules into a new
cql_typesdirectory and creating astatementsdirectory to better organize related functionality - Extracts some session tests from unit tests to integration tests where they can use the public API more appropriately
- Updates Makefile to only update the stable Rust toolchain instead of all toolchains
Reviewed Changes
Copilot reviewed 34 out of 37 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scylla-rust-wrapper/tests/integration/utils.rs | Updates test helper functions with new API signatures and adds utility functions |
| scylla-rust-wrapper/tests/integration/session.rs | New file containing session tests moved from unit tests |
| scylla-rust-wrapper/tests/integration/main.rs | Adds session module to integration tests |
| scylla-rust-wrapper/tests/integration/consistency.rs | Updates to use new test helper API |
| Multiple src/ files | Updates import paths to reflect new module structure with cql_types and statements directories |
| Makefile | Changes rustup update to only update stable toolchain |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| use std::sync::atomic::{AtomicU64, Ordering}; | ||
| use std::time::{SystemTime, UNIX_EPOCH}; | ||
| use uuid::Uuid; | ||
|
|
There was a problem hiding this comment.
The visibility change from pub(crate) to pub for CassUuid should be documented in the PR description as it represents a public API change that exposes this type externally.
Suggested change
| // PUBLIC API CHANGE: CassUuid is now publicly re-exported. | |
| // If this is a new change, ensure it is documented in the PR description as it exposes this type externally. |
Copilot uses AI. Check for mistakes.
Comment on lines
+849
to
851
| pub use crate::cql_types::uuid::{ | ||
| CassUuid, | ||
| cass_uuid_from_string, |
There was a problem hiding this comment.
The addition of CassUuid to the public API exports appears to fix an accidental omission mentioned in the PR description, but this should be verified to ensure all necessary UUID-related types are now properly exposed.
Copilot uses AI. Check for mistakes.
wprzytula
force-pushed
the
refactor-modules
branch
from
95e064d to
775a6cb
Compare
|
Rebased on master. |
It's annoying to have to wait for nightly updates.
There are plenty of top-level modules in the crate that contain CQL types (collection, tuple, user_type, uuid, inet, date_time, value). This change moves them all into a single cql_types module, which should make it easier to find them and understand their purpose. This is a purely organizational change, no logic was changed.
Most of the `cass_types` module is CassDataType-related entities. Those were moved to the newly created `cql_types::data_type` module.
The 3 remaining items there are just public re-exports. They are now moved to other (reasonable) modules, and `cass_types` module is deleted.
These entities are all related to CQL protocol types, so it makes sense to put them there. This annihilates the `misc` module, which is good because the name was not very descriptive.
It makes sense to put all statement-related types (simple statements, prepared statements, and batches) in one module - `statements`.
1. A new module `testing/mod.rs` is created to re-export submodules. 2. `testing.rs` is renamed to `testing/utils.rs`. 3. `integration_testing.rs` is moved to `testing/integration.rs`. 4. `ser_de_tests.rs` is moved to `testing/ser_de_tests.rs`. 5. All imports in the codebase are updated to reflect the new module structure.
ShardAwareness does nothing in dry mode proxy, so we can drop it.
As the code in the closure passed to `test_with_3_node_dry_mode_cluster` is allowed to perform blocking operations (and this often happens when calling C API functions), we need to spawn a blocking task for it. Now, the closure itself is not async anymore, as it is executed in a blocking task. This avoids the pitfall of forgetting to spawn a blocking task inside the closure, which would lead to panics in certain situations.
Although the only existing integration test - consistency.rs - does not benefit from this change (as it must create request rules dynamically), this change makes it easier to write future tests that do not need to create request rules dynamically. It will be used in subsequent commits, when we move more tests from the lib crate to the integration tests.
It's a good practice to have tests which use only the public API in the integration tests, to ensure that the public API is usable and works as expected. Move some session-related tests to the integration tests.
wprzytula
force-pushed
the
refactor-modules
branch
from
775a6cb to
9d1e98d
Compare
Lorak-mmk
approved these changes
Oct 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's done
This PR improves the module structure of the driver by two actions:
Module refactor
Some moduled are moved, grouped, extracted or annihilated, so that the structure makes more sense.
Integration tests extraction
Some tests from
sessionmodule, which only use the public API, are extracted to theintegrationmodule as integration tests.Minor fixes
Makefile
rustupno longer updates all toolchains upon running make commands, just thestabletoolchain.api.rsCassUuid, which must have been accidentally omitted, is now exposed.integrationThe helper function to test with the proxy has now more convenient semantics:
ShardAwareness, which is no-op for the dry mode proxy;tokio::task::spawn_blocking, which is required not to experience panics upon blocking in the async context; the closure itself is no longerasync, which is good - the C API is notasynceither.Pre-review checklist
[ ] I have implemented Rust unit tests for the features/changes introduced.[ ] I have enabled appropriate tests inMakefilein{SCYLLA,CASSANDRA}_(NO_VALGRIND_)TEST_FILTER.[ ] I added appropriateFixes:annotations to PR description.