Assert that redirects go directly to their target location #1814
Conversation
Previously the assertion allowed multiple redirect steps as long as it eventually got to the target. Now it's checked that the very first response directly returns a `Location` header to the final target.
github-actions
bot
added
the
S-waiting-on-review
| @@ -759,7 +763,7 @@ mod test { | |||
| .create() | |||
| .unwrap(); | |||
| let web = env.frontend(); | |||
| assert_redirect("/bat//", "/bat/latest/bat/", web)?; | |||
| assert_redirect_unchecked("/bat//", "/bat/", web)?; | |||
There was a problem hiding this comment.
This redirect is handled by iron, so there's no easy way to make it go directly to the final target.
| @@ -92,6 +92,12 @@ impl<'a> RenderingTimesRecorder<'a> { | |||
|
|
|||
| fn record_current(&mut self) { | |||
| if let Some(current) = self.current.take() { | |||
| #[cfg(test)] | |||
| log::debug!( | |||
| "rendering time - {}: {:?}", | |||
There was a problem hiding this comment.
I added this while trying to figure out why I was getting timeouts from the two reqwest clients deadlocking, but I thought it could be useful to leave in.
| assert_redirect_common(path, expected_target, web) | ||
| } | ||
|
|
||
| /// Make sure that a URL redirects to a specific page, and that the target exists and is not another redirect |
There was a problem hiding this comment.
| /// Make sure that a URL redirects to a specific page, and that the target exists and is not another redirect | |
| /// Make sure that a URL redirects to a specific page, that the target exists and is not another redirect |
(nit)
There was a problem hiding this comment.
I think the double and is more correct, even if a little weird
Make sure (that a URL redirects to a specific page), and (that the target (exists) and (is not another redirect)).
vs
Make sure (that a URL redirects to a specific page), (that the target exists) and (is not another redirect).
| routes.internal_page( | ||
| "/rustc/", | ||
| super::rustdoc::RustLangRedirector::new("nightly", "nightly-rustc"), | ||
| ); |
There was a problem hiding this comment.
it feels odd duplicating this here, but since I'm (still 😅 😉 ) working on the iron/axum rewrite we can leave it :)
There was a problem hiding this comment.
My next PR deletes this whole section 😁
|
LGTM, IMO can be merged with or without fixing the nit/typo :) |
syphar
added
S-waiting-on-author
|
and thank you for this optimization! |
github-actions
bot
added
S-waiting-on-deploy
syphar
removed
the
S-waiting-on-deploy
Previously the assertion allowed multiple redirect steps as long as it eventually got to the target. Now it's checked that the very first response directly returns a
Locationheader to the final target.This should save a couple RTTs for some urls.