Conversation
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gates Failed
(1 hotspot with Large Method)
(1 file with Large Method)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| lib.rs | 1 rule in this hotspot | 10.00 → 9.56 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| lib.rs | 1 advisory rule | 10.00 → 9.56 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThe changes introduce optional PostgreSQL support alongside SQLite for testing utilities, refactor database setup and server launch logic, and update dependency features in several Cargo.toml files. The test utility API now uses database URLs instead of file paths, and related tests are updated accordingly. No changes to exported or public entities in production code. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant TestServer as TestServer
participant DB as Database (SQLite/PostgreSQL)
participant Server as Application Server
Test->>TestServer: start_with_setup(manifest_path, setup_fn)
alt Feature: sqlite
TestServer->>DB: setup_sqlite()
DB-->>TestServer: sqlite db_url
else Feature: postgres
TestServer->>DB: setup_postgres()
DB-->>TestServer: postgres db_url
end
TestServer->>Server: build_server_command(db_url, port)
TestServer->>Server: Start server process
TestServer->>Server: wait_for_server()
Server-->>TestServer: Ready (listening)
TestServer-->>Test: TestServer instance with db_url
Test->>DB: Establish connection using db_url
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
Cargo.toml (1)
39-45:⚠️ Potential issue
test-util/*cannot be referenced from a feature list while it is only a dev-dependency
test-utilis declared exclusively under[dev-dependencies], yet the feature definitions attempt to enable
"test-util/postgres"and"test-util/sqlite".
Cargo will fail when downstream crates enable thepostgresorsqlitefeature because dev-dependencies are not built for normal compilation units.Recommended fix:
- Promote
test-utilto an optional normal dependency:[dependencies] +test-util = { path = "test-util", optional = true, default-features = false }
- Remove the
test-utilentry from[dev-dependencies](or keep it without duplication).This keeps the crate buildable for external consumers while still compiling
test-utilfor tests.Also applies to: 46-52
♻️ Duplicate comments (1)
test-util/src/lib.rs (1)
107-141:⚠️ Potential issueFix compilation errors with feature combinations.
The current implementation has several issues:
- When both
sqliteandpostgresfeatures are enabled,db_urlis defined twice- The
pgvariable is used unconditionally on line 139 but only defined whenpostgresfeature is enabled- The variable shadowing pattern on lines 122-123 is confusing
Additionally, this method remains above the complexity threshold (79 lines vs 70 threshold).
pub fn start_with_setup<F>( manifest_path: &str, setup: F, ) -> Result<Self, Box<dyn std::error::Error>> where F: FnOnce(&str) -> Result<(), Box<dyn std::error::Error>>, { let temp = TempDir::new()?; - #[cfg(feature = "sqlite")] - let db_url = setup_sqlite(&temp, setup)?; - - #[cfg(feature = "postgres")] - let (db_url, mut pg) = setup_postgres(setup)?; - - #[cfg(feature = "postgres")] - let db_url = db_url; + #[cfg(all(feature = "sqlite", not(feature = "postgres")))] + let (db_url, pg) = { + let url = setup_sqlite(&temp, setup)?; + (url, None) + }; + + #[cfg(feature = "postgres")] + let (db_url, pg) = { + let (url, postgres) = setup_postgres(setup)?; + (url, Some(postgres)) + }; + + #[cfg(not(any(feature = "sqlite", feature = "postgres")))] + compile_error!("Either 'sqlite' or 'postgres' feature must be enabled"); let socket = TcpListener::bind("127.0.0.1:0")?; let port = socket.local_addr()?.port(); drop(socket); let mut child = build_server_command(manifest_path, port, &db_url).spawn()?; wait_for_server(&mut child)?; Ok(Self { child, port, db_url, _temp: temp, - #[cfg(feature = "postgres")] - pg: Some(pg), + #[cfg(feature = "postgres")] + pg, + #[cfg(not(feature = "postgres"))] + _pg: std::marker::PhantomData, }) }Also update the struct definition:
pub struct TestServer { child: Child, port: u16, db_url: String, _temp: TempDir, #[cfg(feature = "postgres")] pg: Option<PostgreSQL>, + #[cfg(not(feature = "postgres"))] + _pg: std::marker::PhantomData<()>, }
🧹 Nitpick comments (3)
tests/news_categories.rs (1)
19-25: Minor: favour the existingwith_dbhelper to avoid spinning up ad-hoc runtimesRepeatedly creating a new
tokio::Runtimeinside each test adds a few hundred ms per test and obscures intent.
Thetest_util::with_dbhelper (already used elsewhere) keeps the style uniform and avoids boilerplate.No action is required for correctness, but consider refactoring:
TestServer::start_with_setup("./Cargo.toml", |db| { with_db(db, |conn| { Box::pin(async move { run_migrations(conn).await?; // … }) }) })Also applies to: 107-112, 195-200, 247-253, 313-318
test-util/src/lib.rs (2)
28-36: Remove redundant path-to-string conversions.The function converts the path to string twice with different error handling methods.
fn setup_sqlite<F>(temp: &TempDir, setup: F) -> Result<String, Box<dyn std::error::Error>> where F: FnOnce(&str) -> Result<(), Box<dyn std::error::Error>>, { let path = temp.path().join("mxd.db"); - setup(path.to_str().expect("db path utf8"))?; - Ok(path.to_str().unwrap().to_owned()) + let db_path = path.to_str().ok_or("db path is not valid UTF-8")?; + setup(db_path)?; + Ok(db_path.to_owned()) }
165-169: Consider the runtime creation overhead in Drop.Creating a new Tokio runtime in the Drop implementation adds overhead and could potentially cause issues in some contexts. However, this may be acceptable for test utilities.
Consider documenting this behaviour or exploring alternatives such as:
- Using a shared runtime for PostgreSQL operations
- Making the cleanup synchronous if possible
- Documenting that this struct should not be used in async contexts where runtime creation could be problematic
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.toml(1 hunks)test-util/Cargo.toml(1 hunks)test-util/src/lib.rs(7 hunks)tests/news_articles.rs(1 hunks)tests/news_categories.rs(5 hunks)validator/Cargo.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: coverage
🔇 Additional comments (6)
tests/news_articles.rs (1)
256-259: PossibleString/&strmismatch – double-checkdb_url()return type
DbConnection::establishexpects a&str.
IfTestServer::db_url()returns aString, passing it by value will not auto-coerce and the line will fail to compile; you’d need&server.db_url()(or return&strfromdb_url()).- let mut conn = DbConnection::establish(server.db_url()).await?; + let mut conn = DbConnection::establish(&server.db_url()).await?;Please run
cargo checkto verify the actual signature.validator/Cargo.toml (1)
12-15: LGTM – alignsmxdusage with top-level feature gating
No issues spotted.test-util/src/lib.rs (4)
8-26: LGTM! Clean feature-gated PostgreSQL support.The struct refactoring to use
db_urlinstead ofdb_pathproperly generalises the database connection handling for both SQLite and PostgreSQL.
56-98: Excellent refactoring into helper functions.The extraction of
wait_for_serverandbuild_server_commandimproves code organisation and readability whilst maintaining the original functionality.
148-151: Clean API migration to database URLs.The consistent change from file paths to database URLs across all public APIs properly supports both SQLite and PostgreSQL connection strings.
Also applies to: 206-206, 221-221, 270-270
38-54:Details
✅ Verification successful
Consider the implications of creating a new runtime.
Creating a new Tokio runtime with
block_oncould panic if this function is called from within an existing async runtime. However, this may be acceptable for test utilities that are typically called from synchronous test contexts.
🏁 Script executed:
#!/bin/bash # Check if setup_postgres is ever called from async contexts rg -A 5 'setup_postgres|start_with_setup' --type rustLength of output: 5695
No runtime conflicts detected
All invocations of setup_postgres (via TestServer::start_with_setup) occur in synchronous test functions, so the dedicated Tokio runtime and block_on call will never run inside an existing async context. No changes are required.
| [dev-dependencies] | ||
| test-util = { path = "test-util" } | ||
| test-util = { path = "test-util", default-features = false } | ||
| postgresql_embedded = { version = "0.18.5", features = ["tokio"] } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate declaration after promoting test-util
After the change above, remember to delete the current [dev-dependencies] entry to avoid version/feature mismatches.
-[dev-dependencies]
-test-util = { path = "test-util", default-features = false }
+## entry removed – now in [dependencies] as optionalCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Cargo.toml around lines 58 to 60, there is a duplicate declaration of the
[dev-dependencies] section after promoting the test-util dependency. To fix
this, remove the existing [dev-dependencies] block that contains the test-util
and postgresql_embedded entries to avoid conflicts or mismatches in versions and
features.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
1 participant
Summary
test-utilDiesel deps behind featuresTestServertest-utilTesting
cargo clippy -- -D warningscargo test --no-runcargo testhttps://chatgpt.com/codex/tasks/task_e_6849897e63488322afad1078d5a80215
Summary by Sourcery
Enable optional PostgreSQL in test utilities by feature-gating Diesel dependencies, using database URLs in TestServer, and aligning feature flags across crates
New Features:
postgresfeatureEnhancements:
sqliteandpostgresfeaturessqliteandpostgres) to test-util for consistent configurationBuild:
sqliteandpostgresfeatures for dependency gatingTests:
db_url()and establish connections via URL stringsSummary by CodeRabbit
New Features
Refactor
Chores