-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Conversation
Adds support for testing which route generated a response. This is an implementation of the ideas I put forward in #1878, with a few minor changes. First, I did not create a macro, since my implementation works just fine with a generic type parameter. Writing a macro is pretty trivial, if it's desired. I also added tests to the hello world example to demonstrate what the actual tests look like.
There are a few improvements that could be made:
- The methods added to
LocalResponse(routed_by,caught_byandwas_caught) could probably have better names. - The Request object could (in testing mode) track which routes were attempted, and some basic information about the result. This should be pretty trivial to add, although it may require more that just the route's
TypeIdto actually use this to produce meaningful information.
The only regression I see is that Catcher now has a private member, so other crates cannot directly construct a Catcher, but I think this is acceptable, since it should probably be constructed with the constructor as before.
f0ccdbe to
1400626
Compare
|
It looks like the issue with Windows Debug is a memory or hard drive error. |
|
Indeed, we're running out of disk space. Let's disregard it for now. |
756e34a to
140d1fa
Compare
|
I think I've reached an API I'm pretty happy with. Since the type tracking is done internally, this could be extended to support listing the routes attempted in order, but I don't think that's necessary for this PR. I'm also happy enough with the syntax presented here, I don't feel the need to introduce a new assert macro. @SergioBenitez, this PR is ready for a review, whenever you get a chance. |
cataggar
left a comment
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.
Very cool! This may fix #1878.
| pub(crate) rank: isize, | ||
|
|
||
| /// A unique route type to identify this route | ||
|
pub(crate) catcher_type: Option |
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.
Very cool! I had to read about TypeId.
- RouteType and CatcherType are implemented via codegen for attribute
macros
- Adds routed_by and caught_by methods to local client response
- Adds catcher to RequestState
- Updates route in RequestState to None if a catcher is run
- examples/hello tests now also check which route generated the reponse
- Adds DefaultCatcher type to represent Rocket's default catcher
- FileServer now implements RouteType
- Documents CatcherType, RouteType, and the associated methods in
LocalResponse.
- it terminates on the doctest step with a sigkill. This means I have to
push changes to github to test them.
fe0badf to
09ebdbd
Compare
|
I've rebased on the latest master and push to your fork so that I can review. |
SergioBenitez
left a comment
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.
The way I'm understanding this PR is as making fundamentally two changes:
-
Associate routes and catchers with an ID. Here, the ID is of type
TypeIdwhich is indirect obtained via new traitsCatcherTypeandRouteType, both of which requireAny. The new traits exist so that we can write conditionals of the formresponse.was_routed_by::, wheresuper::worldis a function annotated with#[route]. -
Before a route handler or catcher is called, set state in
Requestindicating the route or catcher. The state can then be queried to check which route or catcher actually handled the request.
On 1: I wish we could simple generate a unique ID whenever Route::new() and Catcher::new() are called and use that instead of introducing a trait that requires manual implementation for custom routes/catchers, and where failing to implement the trait doesn't result in a compile-time error but inhibits functionality. I would really like to be able to avoid these drawbacks while still being able to write something like response.was_routed_by::.
One idea is to convert Route and Catcher into traits. Not only would this resolve the above, but it would also obviate the need for several namespace hacks we have today and pave a way to interact with code generated routes, something sorely missing today. I'm not sure how this approach changes everything else, in particular what custom routes will look like, but it's worth considering.
On 2: I'm concerned about the myriad set() calls sprinkled about. Is there a way to centralize these calls so we can't forget them?
| /// This method is marked as `cfg(test)`, and is therefore only available in unit and | ||
| /// integration tests. This is because the list of routes attempted is only collected in these | ||
| /// testing environments, to minimize performance impacts during normal operation. | ||
| #[cfg(test)] |
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 is unfortunately not the way cfg(test) works. cfg(test) only gets enabled on the top-level crate being tested, not on its dependencies. So rocket compiled as a dependency of an application, even when compiling that application's tests, will never see this cfg enabled.