Add test code for subscribe #283
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #283 +/- ##
===========================================
+ Coverage 10.96% 48.58% +37.62%
===========================================
Files 7 7
Lines 529 531 +2
===========================================
+ Hits 58 258 +200
+ Misses 471 273 -198 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| use super::{Client, Conn, TimeSeries}; | ||
| use super::time_series::clear_last_transfer_time; | ||
| use super::{Client, Conn, REQUIRED_GIGANTO_VERSION, TimeSeries}; |
There was a problem hiding this comment.
Since it's common practice to import super::* in test modules, how about we do that here?
| use super::{Client, Conn, REQUIRED_GIGANTO_VERSION, TimeSeries}; | |
| use super::*; |
| publish_repeat_delay: Duration, | ||
| } | ||
|
|
||
| struct TestServers { |
There was a problem hiding this comment.
I suggest renaming this struct (TestServers).
How about TestServerHandles or TestServerControls?
|
|
||
| #[tokio::test] | ||
| async fn sampling_policy_flow_with_fake_giganto_server() { | ||
| use review_protocol::request::Handler; |
There was a problem hiding this comment.
How about moving this import to the top of the file?
| time, | ||
| &policy, | ||
| event_time, | ||
| &crate::subscribe::Event::Conn(conn_event), |
There was a problem hiding this comment.
If super::* is imported, this can be simplified:
| &crate::subscribe::Event::Conn(conn_event), | |
| &Event::Conn(conn_event), |
| async fn sampling_policy_notify_flow_with_delete() { | ||
| use review_protocol::request::Handler; | ||
|
|
||
| let _lock = TOKEN.lock().await; |
There was a problem hiding this comment.
How about we remove TOKEN and use the serial_test crate instead? (https://crates.io/crates/serial_test)
It seems TOKEN was introduced to prevent concurrent access to the shared object.
| use crate::subscribe::TimeSeries; | ||
| const THREE_MIN: i64 = 3; |
There was a problem hiding this comment.
How about moving this import and constant definition to the top of the file?
| timeout(Duration::from_secs(5), notify_recv.recv()) | ||
| .await | ||
| .expect("ingest notify timeout") | ||
| .expect("ingest notify recv"); |
There was a problem hiding this comment.
(1) The expect() messages seem to differ from our convention. We usually describe the expected result.
(2) How about adding an assertion to verify that the received value matches the policy ID?
| timeout(Duration::from_secs(5), notify_recv.recv()) | |
| .await | |
| .expect("ingest notify timeout") | |
| .expect("ingest notify recv"); | |
| let id = timeout(Duration::from_secs(5), notify_recv.recv()) | |
| .await | |
| .expect("Received ingest notify before timeout") | |
| .expect("Policy ID"); | |
| assert_eq!(id, policy.id); |
| if last_time_series_path.exists() { | ||
| let content = fs::read_to_string(&last_time_series_path).unwrap_or_default(); | ||
| eprintln!("time_data.json content: {content}"); | ||
| } |
There was a problem hiding this comment.
I noticed several eprintln! calls throughout the tests, both inside and outside of if blocks.
Are these intended for temporary debugging?
If they are meant to provide context for failures, I suggest refactoring them into the assert! or assert_eq! macros as custom messages.
This would make the code cleaner and more idiomatic.
Close: #267
Added new integration tests:
sampling_policy_flow_with_fake_giganto_server: Validates the basic flow of policy registration and data reception.sampling_policy_notify_flow_with_delete: Verifies stream termination and local file cleanup upon policy deletion.sampling_policy_multiple_streams: Ensures individual streams operate correctly when multiple policies are active simultaneously.Enhanced test readability:
timeseries_with_conn: Now validates that multiple Conn events within the same 15-minute slot are correctly aggregated.