Skip to content

Create a new modelardb_test crate for test functionality - #337

Merged
CGodiksen merged 13 commits into
mainfrom
dev/modelardb_test
Jul 9, 2025
Merged

Create a new modelardb_test crate for test functionality#337
CGodiksen merged 13 commits into
mainfrom
dev/modelardb_test

Conversation

@CGodiksen

Copy link
Copy Markdown
Collaborator

This PR implements #260 by creating a new modelardb_test crate for shared test functionality. modelardb_common has also been removed since most of the code that was left in modelardb_common after recent changes was test util.

Note that some functionality was duplicated when moved out of modelardb_common and into modelardb_manager and modelardb_server. This is not considered a problem since modelardb_manager will be removed.

@CGodiksen CGodiksen self-assigned this Jul 6, 2025
@CGodiksen CGodiksen linked an issue Jul 6, 2025 that may be closed by this pull request
@CGodiksen
CGodiksen requested a review from Copilot July 6, 2025 18:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements issue #260 by replacing the deprecated modelardb_common crate with a new modelardb_test crate for shared testing utilities, updating imports and dependencies across all crates, and adding local command-line argument helpers in the server and manager binaries.

  • Created modelardb_test crate and moved test constants/functions into it
  • Removed modelardb_common references and updated imports throughout the workspace
  • Added collect_command_line_arguments and print_usage_and_exit_with_error in server and manager binaries

Reviewed Changes

Copilot reviewed 44 out of 45 changed files in this pull request and generated no comments.

Show a summary per file
File Description
docs/dev/README.md Removed modelardb_common section and added modelardb_test entry
crates/modelardb_types/src/types.rs Switched test imports to use modelardb_test constants and functions
crates/modelardb_test/src/lib.rs Introduced table module and updated crate metadata for modelardb_test
crates/modelardb_storage/src/metadata/table_metadata_manager.rs Removed modelardb_common and replaced ERROR_BOUND_ZERO constant
crates/modelardb_server/src/main.rs Removed modelardb_common args, added local CLI helpers
crates/modelardb_server/src/remote.rs Added Flight helpers and removed modelardb_common remote imports
crates/modelardb_manager/src/main.rs Removed modelardb_common args, added local CLI helpers
Comments suppressed due to low confidence (5)

crates/modelardb_server/src/main.rs:126

  • [nitpick] Public helper functions should have doc comments explaining their behavior and parameters for improved maintainability.
pub fn print_usage_and_exit_with_error(parameters: &str) -> ! {

crates/modelardb_server/src/remote.rs:240

  • [nitpick] Returning &String may be less ergonomic than &str; consider returning &str to simplify the API for callers.
pub fn table_name_from_flight_descriptor(

crates/modelardb_storage/src/metadata/table_metadata_manager.rs:544

  • [nitpick] Consider using the ERROR_BOUND_ZERO constant from modelardb_test instead of the literal 0.0 to avoid a magic number and maintain consistency.
            vec![ErrorBound::try_new_absolute(0.0)?; query_schema_columns];

crates/modelardb_server/src/main.rs:114

  • [nitpick] This function is duplicated in modelardb_manager; consider extracting argument parsing utilities into a shared module or crate to reduce duplication.
pub fn collect_command_line_arguments(maximum_arguments: usize) -> Vec<String> {

crates/modelardb_server/src/remote.rs:258

  • [nitpick] The error_to_status_* helpers are duplicated in modelardb_manager; extracting them to a shared utility module would reduce code duplication.
pub fn error_to_status_invalid_argument(error: impl Error) -> Status {

@CGodiksen
CGodiksen requested a review from skejserjensen July 6, 2025 18:56
Comment thread crates/modelardb_test/Cargo.toml
@skejserjensen
skejserjensen requested a review from chrthomsen July 7, 2025 09:06
@CGodiksen
CGodiksen merged commit 66a4abf into main Jul 9, 2025
4 checks passed
@CGodiksen
CGodiksen deleted the dev/modelardb_test branch July 9, 2025 07:51
aabduvakhobov pushed a commit to aabduvakhobov/ModelarDB-RS that referenced this pull request Aug 4, 2025
…#337)

* Add modelardb_test crate

* Move modelardb_common test constants to modelardb_test

* Move data generation test util to modelardb_test crate

* Move modelardb_storage test util to modelardb_test

* Move argument util functions to manager and server

* Move remote util functions to manager and server

* Remove modelardb_common crate

* Fix Clippy issues

* Update import and return type in moved functions

* Fix broken link in dev docs

* Add text about imports to dev docs

* Update imports according to new style guide

* Update copyright year for files that were moved
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.

Create a modelardb_test crate for test functionality

4 participants