-
Notifications
You must be signed in to change notification settings - Fork 13
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.
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
base: main
Are you sure you want to change the base?
Conversation
- Fix unresolved link warnings by using conditional doc attributes - Improve Key Concepts section with clearer explanations - Enhance Layers and Middleware example with better comments - Move public API tests to tests/ directory - Keep internal tests in source modules Co-authored-by: martintmk <103487740+martintmk@users.noreply.github.com>
Co-authored-by: martintmk <103487740+martintmk@users.noreply.github.com>
| assert_eq!(block_on(cloned.execute(1)), 1); | ||
| assert_eq!(format!("{svc:?}"), "DynamicService"); | ||
| } | ||
| // Integration tests have been moved to tests/dynamic_service.rs |
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.
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)
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.
I'd love to know that too...
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.
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
srcif 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 oursrcmore readable.
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.
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.
Fixes rustdoc warnings for feature-gated types and improves clarity of core concepts documentation.
Documentation improvements
Unresolved link warnings: Conditional
cfg_attrattributes now prevent rustdoc warnings when features are disabled. Links toIntercept,DynamicService, andtowermodule resolve correctly based on active features.Key Concepts section: Clarified that Service is a trait implementation (not "an async function"), middleware is a service wrapper (not just "adds behavior"), and Layer is a trait that constructs middleware (removed "factory" terminology).
Layers and Middleware example: Added explicit comment identifying the middleware service wrapper to make the pattern concrete.
Test organization
Moved public API tests to
tests/directory:Internal tests using
MockServiceor testing implementation details remain in source modules.Changes summary
lib.rs: Feature doc links now conditional on feature flagsREADME.md: Synchronized with lib.rs documentation updatestests/*.rs: Five new integration test files covering public APIsOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.