chore: benchmark bin relocation + streaming smoke test + addendum update #42
Conversation
Summary of ChangesHello @zensgit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the development and testing infrastructure for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR performs maintenance tasks by relocating benchmark scripts, adding feature-gated tests, and updating documentation. The changes improve the project's organization and provide better testing coverage for the streaming export feature.
- Relocates benchmark script to a proper bin location for easier execution
- Adds a smoke test for streaming export functionality with proper feature gating
- Updates documentation with precise benchmark execution instructions
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| jive-api/tests/integration/transactions_export_stream_smoke_test.rs | Adds feature-gated integration test for streaming CSV export functionality |
| docs/PR_MERGE_REPORT_2025_09_25_ADDENDUM.md | Updates benchmark instructions with relocated script path and detailed curl comparison steps |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| async fn streaming_export_smoke() { | ||
| let pool = create_test_pool().await; | ||
| let auth = AuthService::new(pool.clone()); | ||
| let user_ctx = auth.register_with_family(RegisterRequest { email: format!("stream_{}@example.com", Uuid::new_v4()), password: "Stream123!".into(), name: Some("Streamer".into()), username: None }).await.expect("register"); |
There was a problem hiding this comment.
[nitpick] This line is extremely long and hard to read. Consider breaking it into multiple lines or extracting the RegisterRequest creation into a separate variable for better readability.
| let user_ctx = auth.register_with_family(RegisterRequest { email: format!("stream_{}@example.com", Uuid::new_v4()), password: "Stream123!".into(), name: Some("Streamer".into()), username: None }).await.expect("register"); | |
| let register_req = RegisterRequest { | |
| email: format!("stream_{}@example.com", Uuid::new_v4()), | |
| password: "Stream123!".into(), | |
| name: Some("Streamer".into()), | |
| username: None, | |
| }; | |
| let user_ctx = auth.register_with_family(register_req).await.expect("register"); |
| sqlx::query("INSERT INTO accounts (id,ledger_id,name,account_type,currency,current_balance,created_at,updated_at) VALUES ($1,$2,'SAcc','cash','CNY',0,NOW(),NOW())") | ||
| .bind(account_id).bind(ledger_id.0).execute(&pool).await.expect("account"); | ||
| sqlx::query("INSERT INTO transactions (id,ledger_id,account_id,transaction_type,amount,currency,transaction_date,description,created_at,updated_at) VALUES ($1,$2,$3,'expense',10,'CNY',CURRENT_DATE,'Test',NOW(),NOW())") | ||
| .bind(Uuid::new_v4()).bind(ledger_id.0).bind(account_id).execute(&pool).await.expect("txn"); |
There was a problem hiding this comment.
[nitpick] These raw SQL queries contain magic values ('SAcc', 'cash', 'CNY', 'expense', 10, 'Test') that make the test harder to understand and maintain. Consider extracting these as named constants or using more descriptive values that clearly indicate this is test data.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively relocates the benchmark script, adds a new smoke test for the streaming export feature, and updates the related documentation. The changes are well-focused. My review includes suggestions to improve the robustness and correctness of the new benchmark script and to strengthen the assertions in the new integration test.
| while let Some(a) = args.next() { | ||
| match a.as_str() { | ||
| "--rows" => if let Some(v) = args.next() { rows = v.parse().unwrap_or(rows); }, | ||
| "--database-url" => if let Some(v) = args.next() { db_url = v; }, | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
The current manual argument parsing logic is buggy. If a flag like --rows is provided without a value, it incorrectly consumes the next argument (which could be another flag like --database-url) as its value. This causes parsing to fail silently, the script to run with default values, and the next flag to be skipped. This can lead to incorrect benchmark runs and is hard to debug.
For robust CLI argument parsing, it's highly recommended to use a library like clap.
Example with clap:
// Add to Cargo.toml: clap = { version = "4", features = ["derive", "env"] }
use clap::Parser;
#[derive(Parser)]
struct Cli {
#[arg(long, default_value_t = 5000)]
rows: i64,
#[arg(long, env = "DATABASE_URL")]
database_url: String,
}
// in main()
let cli = Cli::parse();
let rows = cli.rows;
let db_url = cli.database_url;
// ...| let mut sep = qb.separated(","); | ||
| for _ in 0..take { | ||
| let id = uuid::Uuid::new_v4(); | ||
| let amount = Decimal::from_f64(rng.gen_range(1.0..500.0)).unwrap(); |
There was a problem hiding this comment.
Using .unwrap() on the result of Decimal::from_f64 can lead to a panic if the f64 value is infinite or NaN. While the random range 1.0..500.0 makes this unlikely, it's safer to use .expect() with a descriptive message to make debugging easier in case of a panic.
| let amount = Decimal::from_f64(rng.gen_range(1.0..500.0)).unwrap(); | |
| let amount = Decimal::from_f64(rng.gen_range(1.0..500.0)).expect("f64 to Decimal conversion failed"); |
| let resp = app.clone().oneshot(req).await.unwrap(); | ||
| assert_eq!(resp.status(), StatusCode::OK); | ||
| let body_bytes = hyper::body::to_bytes(resp.into_body()).await.unwrap(); | ||
| assert!(body_bytes.starts_with(b"Date,Description"), "CSV header missing or incorrect"); |
There was a problem hiding this comment.
The smoke test assertion for the CSV header is a bit weak as it only checks for the first two columns (Date,Description). This could allow regressions in other parts of the header to go unnoticed. To make the test more robust, it should assert the presence of the full header.
| assert!(body_bytes.starts_with(b"Date,Description"), "CSV header missing or incorrect"); | |
| assert!(body_bytes.starts_with(b"Date,Description,Amount,Category,Account,Payee,Type\n"), "CSV header missing or incorrect"); |
1 participant
Changes:
Notes:
All tests pass with feature: SQLX_OFFLINE=true cargo test --features export_stream --tests.