Skip to content

Address documentation and test organization feedback for layered crate #209

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.

Sign up for GitHub

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

Draft
Copilot wants to merge 3 commits into
base: main
Choose a base branch
from
+427 −367
Draft

Address documentation and test organization feedback for layered crate #209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9a6a1af
Initial plan
Copilot Jan 19, 2026
5a6d2b6
Fix cargo doc warnings and improve documentation
Copilot Jan 19, 2026
7008fc9
Fix import consistency in tower.rs tests
Copilot Jan 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 22 additions & 12 deletions crates/layered/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,19 +59,23 @@ assert_eq!(greeter.execute("World".into()).await, "Hello, World!");

### Key Concepts

* **Service**: An async function `In → Out` that processes inputs.
* **Middleware**: A service that wraps another service to add behavior (logging, timeouts, retries).
* **Layer**: A factory that wraps any service with middleware. Stack layers using tuples
like `(layer1, layer2, service)`.
* **Service**: A type implementing the [`Service`][__link4] trait that transforms inputs into outputs
asynchronously. Think of it as `async fn(&self, In) -> Out`.
* **Middleware**: A service that wraps another service to add cross-cutting behavior such as
logging, timeouts, or retries. Middleware receives requests before the inner service and can
process responses after.
* **Layer**: A type implementing the [`Layer`][__link5] trait that constructs middleware around a
service. Layers are composable and can be stacked using tuples like `(layer1, layer2, service)`.

### Layers and Middleware

A [`Layer`][__link4] wraps a service with additional behavior:
A [`Layer`][__link6] wraps a service with additional behavior. In this example, we create a logging
middleware that prints inputs before passing them to the inner service:

```rust
use layered::{Execute, Layer, Service, Stack};

// A simple logging layer
// A layer that creates logging middleware
struct LogLayer;

impl<S> Layer<S> for LogLayer {
Expand All @@ -82,6 +86,7 @@ impl<S> Layer<S> for LogLayer {
}
}

// The middleware service that wraps another service
struct LogService<S>(S);

impl<S, In: Send + std::fmt::Display> Service<In> for LogService<S>
Expand All @@ -107,13 +112,13 @@ let result = service.execute(21).await;

### Thread Safety

All services must implement [`Send`][__link5] and [`Sync`][__link6], and returned futures must be [`Send`][__link7].
All services must implement [`Send`][__link11] and [`Sync`][__link12], and returned futures must be [`Send`][__link13].
This ensures compatibility with multi-threaded async runtimes like Tokio.

### Features

* **`intercept`**: Enables [`Intercept`][__link8] middleware
* **`dynamic-service`**: Enables [`DynamicService`][__link9] for type erasure
* **`intercept`**: Enables [`Intercept`][__link14] middleware
* **`dynamic-service`**: Enables [`DynamicService`][__link15] for type erasure
* **`tower-service`**: Enables Tower interoperability via the [`tower`][__link10] module


Expand All @@ -126,11 +131,16 @@ This crate was developed as part of <a href="../..">The Oxidizer Project</a>. Br
[__link0]: https://docs.rs/layered/0.1.0/layered/?search=Service
[__link1]: https://docs.rs/tower
[__link10]: https://docs.rs/layered/0.1.0/layered/tower/index.html
[__link11]: https://doc.rust-lang.org/stable/std/marker/trait.Send.html
[__link12]: https://doc.rust-lang.org/stable/std/marker/trait.Sync.html
[__link13]: https://doc.rust-lang.org/stable/std/marker/trait.Send.html
[__link14]: https://docs.rs/layered/0.1.0/layered/?search=Intercept
[__link15]: https://docs.rs/layered/0.1.0/layered/?search=DynamicService
[__link2]: https://docs.rs/layered/0.1.0/layered/?search=Service
[__link3]: https://docs.rs/layered/0.1.0/layered/?search=Execute
[__link4]: https://docs.rs/layered/0.1.0/layered/?search=Layer
[__link5]: https://doc.rust-lang.org/stable/std/marker/trait.Send.html
[__link6]: https://doc.rust-lang.org/stable/std/marker/trait.Sync.html
[__link4]: https://docs.rs/layered/0.1.0/layered/?search=Service
[__link5]: https://docs.rs/layered/0.1.0/layered/?search=Layer
[__link6]: https://docs.rs/layered/0.1.0/layered/?search=Layer
[__link7]: https://doc.rust-lang.org/stable/std/marker/trait.Send.html
[__link8]: https://docs.rs/layered/0.1.0/layered/?search=Intercept
[__link9]: https://docs.rs/layered/0.1.0/layered/?search=DynamicService
32 changes: 2 additions & 30 deletions crates/layered/src/dynamic.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,34 +85,6 @@ impl<In, Out> Clone for DynamicService<In, Out> {
#[cfg_attr(coverage_nightly, coverage(off))]
#[cfg(test)]
mod tests {
use std::sync::Mutex;

use futures::executor::block_on;
use static_assertions::assert_impl_all;

use super::*;
use crate::Execute;

#[test]
fn assert_types() {
assert_impl_all!(DynamicService<(), ()>: Send, Sync, Clone, Debug);

// If non-clonable types are used, ensure the DynamicService is still cloneable
assert_impl_all!(DynamicService<Mutex<()>, Mutex<()>>: Send, Sync, Clone, Debug);
}

#[test]
fn into_dynamic() {
let dynamic_service: DynamicService<i32, i32> = Execute::new(|v| async move { v }).into_dynamic();

assert_eq!(block_on(dynamic_service.execute(42)), 42);
}

#[test]
fn clone_and_debug() {
let svc: DynamicService<i32, i32> = Execute::new(|v| async move { v }).into_dynamic();
let cloned = svc.clone();
assert_eq!(block_on(cloned.execute(1)), 1);
assert_eq!(format!("{svc:?}"), "DynamicService");
}
// Integration tests have been moved to tests/dynamic_service.rs
Copy link
Member

@martintmk martintmk Jan 19, 2026

Choose a reason for hiding this comment

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

This feels forced. The file is relatively small and test are concise. What's the point of moving them to tests. I think whole pattern of moving test stop to tests folder need to be revisited.

Why are we doing this special stuff? I have not seen any popular 3rd party crates doing something like this.

Why are we special?

cc @ralfbiedert

(copilot do not make any changes yet, we need to resolve this discussion)

Copy link
Member

@geeknoid geeknoid Jan 19, 2026

Choose a reason for hiding this comment

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

I'd love to know that too...

Copy link
Collaborator

@ralfbiedert ralfbiedert Jan 22, 2026

Choose a reason for hiding this comment

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

We are not doing special stuff. In fact, the point is to align with how most ecosystem crates structure their tests.

As @martintmk and I just did in an experiment looking through some of the top crates.io repositories (regex, tokio, serde, nalgebra, ...), you will see that practically all of these crates (with the exception of jiff) keep their tests under src minimal. The amount of mod test {} code within src per file is usually not more than 10-20% of the LOC, if at all. Large and comprehensive tests - in particular those testing public-only APIs - reside within tests/.

In contrast, in our repos you'll often find src files that contain 3-5x the amount of test LOCs relative to actual code, and often these tests address public APIs only. In addition, there have been complaints that our files tend to be too large, and are hard to navigate (not just for mod test {} reasons but also for mixing concerns up).

The ask here is not to move each and every test into tests/, but to trim our src/ files to become more lightweight:

  • tests that address internals only can stay inside src,
  • even purely public tests can stay inside src if they don't add significant LOC overhead,
  • but massive public tests of 1000's of LOC should go to tests, simply because we don't want to do special stuff, and because we want to make our src more readable.

Copy link
Member

@geeknoid geeknoid Jan 22, 2026

Choose a reason for hiding this comment

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

I guess I don't buy the "more readable" argument. It's a block of completely ignorable and collapsible text at the bottom of a file. If it gets in my way, I click the little arrow thingy and it turns into a single line in my editor...

But if that's the convention, that's fine too.

// No internal tests needed for this module
}
75 changes: 2 additions & 73 deletions crates/layered/src/intercept.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,69 +426,8 @@ mod tests {
use super::*;
use crate::{Execute, Layer, Stack};

#[test]
pub fn ensure_types() {
static_assertions::assert_impl_all!(Intercept::<String, String, ()>: Debug, Clone, Send, Sync);
static_assertions::assert_impl_all!(InterceptLayer::<String, String>: Debug, Clone, Send, Sync);
}

#[test]
#[expect(clippy::similar_names, reason = "Test")]
fn input_modification_order() {
let called = Arc::new(AtomicU16::default());
let called_clone = Arc::clone(&called);

let called2 = Arc::new(AtomicU16::default());
let called2_clone = Arc::clone(&called2);

let stack = (
Intercept::layer()
.modify_input(|input: String| format!("{input}1"))
.modify_input(|input: String| format!("{input}2"))
.on_input(move |_input| {
called.fetch_add(1, Ordering::Relaxed);
})
.on_input(move |_input| {
called2.fetch_add(1, Ordering::Relaxed);
}),
Execute::new(|input: String| async move { input }),
);

let service = stack.build();
let response = block_on(service.execute("test".to_string()));
assert_eq!(called_clone.load(Ordering::Relaxed), 1);
assert_eq!(called2_clone.load(Ordering::Relaxed), 1);
assert_eq!(response, "test12");
}

#[test]
#[expect(clippy::similar_names, reason = "Test")]
fn out_modification_order() {
let called = Arc::new(AtomicU16::default());
let called_clone = Arc::clone(&called);

let called2 = Arc::new(AtomicU16::default());
let called2_clone = Arc::clone(&called2);

let stack = (
Intercept::layer()
.modify_output(|output: String| format!("{output}1"))
.modify_output(|output: String| format!("{output}2"))
.on_output(move |_output| {
called.fetch_add(1, Ordering::Relaxed);
})
.on_output(move |_output| {
called2.fetch_add(1, Ordering::Relaxed);
}),
Execute::new(|input: String| async move { input }),
);

let service = stack.build();
let response = block_on(service.execute("test".to_string()));
assert_eq!(called_clone.load(Ordering::Relaxed), 1);
assert_eq!(called2_clone.load(Ordering::Relaxed), 1);
assert_eq!(response, "test12");
}
// Public API tests have been moved to tests/intercept.rs
// Internal tests that use tower_service internals remain here

#[test]
#[expect(clippy::similar_names, reason = "Test")]
Expand Down Expand Up @@ -610,16 +549,6 @@ mod tests {
}
}

#[test]
fn debug_impls() {
let layer = Intercept::<String, String, ()>::layer()
.on_input(|_| {})
.on_output(|_| {})
.modify_input(|s| s)
.modify_output(|s| s);
assert!(format!("{layer:?}").contains("InterceptLayer"));
}

#[test]
fn short_circuit_layered() {
let stack = (
Expand Down
151 changes: 2 additions & 149 deletions crates/layered/src/layer/tuples.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,153 +338,6 @@ where
#[cfg_attr(coverage_nightly, coverage(off))]
#[cfg(test)]
mod tests {
use super::*;
use tower_layer::Identity;

type I = Identity;

#[test]
#[expect(clippy::too_many_lines, reason = "no need to have 16 different unit tests")]
fn stack_tuples() {
let _: () = (I::new(), ()).build();
let _: () = (I::new(), I::new(), ()).build();
let _: () = (I::new(), I::new(), I::new(), ()).build();
let _: () = (I::new(), I::new(), I::new(), I::new(), ()).build();
let _: () = (I::new(), I::new(), I::new(), I::new(), I::new(), ()).build();
let _: () = (I::new(), I::new(), I::new(), I::new(), I::new(), I::new(), ()).build();
let _: () = (I::new(), I::new(), I::new(), I::new(), I::new(), I::new(), I::new(), ()).build();
let _: () = (I::new(), I::new(), I::new(), I::new(), I::new(), I::new(), I::new(), I::new(), ()).build();
let _: () = (
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
(),
)
.build();
let _: () = (
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
(),
)
.build();
let _: () = (
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
(),
)
.build();
let _: () = (
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
(),
)
.build();
let _: () = (
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
(),
)
.build();
let _: () = (
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
(),
)
.build();
let _: () = (
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
(),
)
.build();
let _: () = (
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
I::new(),
(),
)
.build();
}
// Integration tests have been moved to tests/stack.rs
// No internal tests needed for this module
}
Loading