-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor test helpers #241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| //! Shared utilities for integration tests. | ||
| //! | ||
| //! Provides fixtures for a basic [`WireframeApp`] factory and an unused | ||
| //! local port. These helpers reduce duplication across test modules. | ||
|
|
||
| use std::net::{Ipv4Addr, SocketAddr, TcpListener as StdTcpListener}; | ||
|
|
||
| /// Create a TCP listener bound to a free local port. | ||
| pub fn unused_listener() -> StdTcpListener { | ||
| let addr = SocketAddr::new(Ipv4Addr::LOCALHOST.into(), 0); | ||
| StdTcpListener::bind(addr).expect("failed to bind port") | ||
| } | ||
|
|
||
| use rstest::fixture; | ||
| use wireframe::app::WireframeApp; | ||
|
|
||
| #[fixture] | ||
| #[allow( | ||
| unused_braces, | ||
| reason = "rustc false positive for single line rstest fixtures" | ||
| )] | ||
| pub fn factory() -> impl Fn() -> WireframeApp + Send + Sync + Clone + 'static { | ||
| || WireframeApp::new().expect("WireframeApp::new failed") | ||
| } | ||
|
|
||
| #[fixture] | ||
| #[allow( | ||
| unused_braces, | ||
| reason = "rustc false positive for single line rstest fixtures" | ||
| )] | ||
| pub fn unused_port() -> SocketAddr { | ||
| unused_listener() | ||
| .local_addr() | ||
| .expect("failed to obtain local addr") | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,7 +4,9 @@ use std::io; | |||||||||||
|
|
||||||||||||
| use bincode::error::DecodeError; | ||||||||||||
| use futures::future::BoxFuture; | ||||||||||||
| use rstest::{fixture, rstest}; | ||||||||||||
| mod common; | ||||||||||||
| use common::{factory, unused_listener}; | ||||||||||||
| use rstest::rstest; | ||||||||||||
| use tokio::{ | ||||||||||||
| io::{AsyncReadExt, AsyncWriteExt, duplex}, | ||||||||||||
| net::TcpStream, | ||||||||||||
|
|
@@ -34,11 +36,6 @@ impl HotlinePreamble { | |||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| #[fixture] | ||||||||||||
| fn factory() -> impl Fn() -> WireframeApp + Send + Sync + Clone + 'static { | ||||||||||||
| || WireframeApp::new().expect("WireframeApp::new failed") | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /// Create a server configured with `HotlinePreamble` handlers. | ||||||||||||
| fn server_with_handlers<F, S, E>( | ||||||||||||
| factory: F, | ||||||||||||
|
|
@@ -68,9 +65,9 @@ where | |||||||||||
| Fut: std::future::Future<Output = ()>, | ||||||||||||
| B: FnOnce(std::net::SocketAddr) -> Fut, | ||||||||||||
| { | ||||||||||||
| let server = server | ||||||||||||
| .bind("127.0.0.1:0".parse().expect("hard-coded socket addr")) | ||||||||||||
| .expect("bind"); | ||||||||||||
| let listener = unused_listener(); | ||||||||||||
| let _addr = listener.local_addr().expect("addr"); | ||||||||||||
| let server = server.bind_listener(listener).expect("bind"); | ||||||||||||
|
Comment on lines
+68
to
+70
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Remove unused variable assignment. Line 69 assigns - let listener = unused_listener();
- let _addr = listener.local_addr().expect("addr");
- let server = server.bind_listener(listener).expect("bind");
+ let listener = unused_listener();
+ let server = server.bind_listener(listener).expect("bind");📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||
| let addr = server.local_addr().expect("addr"); | ||||||||||||
| let (shutdown_tx, shutdown_rx) = oneshot::channel::<()>(); | ||||||||||||
| let handle = tokio::spawn(async move { | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,31 +1,31 @@ | ||||||||||||||||||||||||||||||||||||
| //! Tests for [`WireframeServer`] configuration. | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| use wireframe::{app::WireframeApp, server::WireframeServer}; | ||||||||||||||||||||||||||||||||||||
| mod common; | ||||||||||||||||||||||||||||||||||||
| use common::{factory, unused_listener}; | ||||||||||||||||||||||||||||||||||||
| use wireframe::server::WireframeServer; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||
| fn default_worker_count_matches_cpu_count() { | ||||||||||||||||||||||||||||||||||||
| let server = WireframeServer::new(|| WireframeApp::new().expect("WireframeApp::new failed")); | ||||||||||||||||||||||||||||||||||||
| let server = WireframeServer::new(factory()); | ||||||||||||||||||||||||||||||||||||
| let expected = std::thread::available_parallelism().map_or(1, std::num::NonZeroUsize::get); | ||||||||||||||||||||||||||||||||||||
| assert_eq!(server.worker_count(), expected); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||
| fn default_workers_at_least_one() { | ||||||||||||||||||||||||||||||||||||
| let server = WireframeServer::new(|| WireframeApp::new().expect("WireframeApp::new failed")); | ||||||||||||||||||||||||||||||||||||
| let server = WireframeServer::new(factory()); | ||||||||||||||||||||||||||||||||||||
| assert!(server.worker_count() >= 1); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||
| fn workers_method_enforces_minimum() { | ||||||||||||||||||||||||||||||||||||
| let server = | ||||||||||||||||||||||||||||||||||||
| WireframeServer::new(|| WireframeApp::new().expect("WireframeApp::new failed")).workers(0); | ||||||||||||||||||||||||||||||||||||
| let server = WireframeServer::new(factory()).workers(0); | ||||||||||||||||||||||||||||||||||||
| assert_eq!(server.worker_count(), 1); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||
| fn workers_accepts_large_values() { | ||||||||||||||||||||||||||||||||||||
| let server = WireframeServer::new(|| WireframeApp::new().expect("WireframeApp::new failed")) | ||||||||||||||||||||||||||||||||||||
| .workers(128); | ||||||||||||||||||||||||||||||||||||
| let server = WireframeServer::new(factory()).workers(128); | ||||||||||||||||||||||||||||||||||||
| assert_eq!(server.worker_count(), 128); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
|
|
@@ -39,15 +39,12 @@ async fn readiness_receiver_dropped() { | |||||||||||||||||||||||||||||||||||
| time::{Duration, sleep}, | ||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| let factory = || WireframeApp::new().expect("WireframeApp::new failed"); | ||||||||||||||||||||||||||||||||||||
| let server = WireframeServer::new(factory) | ||||||||||||||||||||||||||||||||||||
| let listener = unused_listener(); | ||||||||||||||||||||||||||||||||||||
| let _addr = listener.local_addr().unwrap(); | ||||||||||||||||||||||||||||||||||||
| let server = WireframeServer::new(factory()) | ||||||||||||||||||||||||||||||||||||
| .workers(1) | ||||||||||||||||||||||||||||||||||||
| .bind( | ||||||||||||||||||||||||||||||||||||
| "127.0.0.1:0" | ||||||||||||||||||||||||||||||||||||
| .parse() | ||||||||||||||||||||||||||||||||||||
| .expect("hard-coded socket address must be valid"), | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| .expect("bind failed"); | ||||||||||||||||||||||||||||||||||||
| .bind_listener(listener) | ||||||||||||||||||||||||||||||||||||
| .unwrap(); | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+42
to
+47
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Remove unused variable assignment. Line 43 assigns - let listener = unused_listener();
- let _addr = listener.local_addr().unwrap();
- let server = WireframeServer::new(factory())
+ let listener = unused_listener();
+ let server = WireframeServer::new(factory())
.workers(1)
.bind_listener(listener)
- .unwrap();
+ .unwrap();📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| let addr = server.local_addr().expect("local addr missing"); | ||||||||||||||||||||||||||||||||||||
| // Create channel and immediately drop receiver to force send failure | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential race condition in unused_port fixture.
The
unused_portfixture creates a listener, extracts its address, then drops the listener. This introduces a race condition where another process could bind to the same port before the test uses it.Replace the fixture with direct usage of
unused_listener()in tests that need a bound listener, or ensure the listener remains alive until the test binds to it.🤖 Prompt for AI Agents