servo: Merge #9391 - Test redirect_count boundaries in Fetch (from nikkisquared:test_redirect); r=jdm

I've written two new tests for Fetch: one to test the highest possible number of redirects succeeds; and another to ensure a failure in Fetch by requesting too many redirects. I also wrote a helper function to be used by each test, since the main difference is how many times they try to redirect.

I've also changed the check against redirect_count in http_network fetch to compare it as greater than or equal to 20, as opposed to being only equal to 20. That's outside of the spec, but in my experience testing for pure equality can easily create errors. Even though it's technically not possible for redirect_count be above 20, bizarre bugs during runtime certainly happen.

Source-Repo: https://github.com/servo/servo
Source-Revision: c80fa3386459efd27b64c8b6cab33794e66d082b

servo/components/net/fetch/methods.rs

@@ -298,7 +298,7 @@ fn http_fetch(request: Rc<Request>,
             };
 
             // Step 7
-            if request.redirect_count.get() == 20 {
+            if request.redirect_count.get() >= 20 {
                 return Response::network_error();
             }
 

servo/tests/unit/net/fetch.rs

@@ -2,19 +2,20 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-use hyper::server::{Listening, Server};
+use hyper::header::{Location};
+use hyper::server::{Handler, Listening, Server};
 use hyper::server::{Request as HyperRequest, Response as HyperResponse};
+use hyper::status::StatusCode;
+use hyper::uri::RequestUri;
 use net::fetch::methods::fetch;
 use net_traits::request::{Context, Referer, Request};
 use net_traits::response::{Response, ResponseBody};
 use std::rc::Rc;
 use url::Url;
 
-fn make_server(message: &'static [u8]) -> (Listening, Url) {
+// TODO write a struct that impls Handler for storing test values
 
-    let handler = move |_: HyperRequest, response: HyperResponse| {
-        response.send(message).unwrap();
-    };
+fn make_server<H: Handler + 'static>(handler: H) -> (Listening, Url) {
 
     // this is a Listening server because of handle_threads()
     let server = Server::http("0.0.0.0:0").unwrap().handle_threads(handler, 1).unwrap();
@@ -29,7 +30,10 @@ fn make_server(message: &'static [u8]) -> (Listening, Url) {
 fn test_fetch_response_is_not_network_error() {
 
     static MESSAGE: &'static [u8] = b"";
-    let (mut server, url) = make_server(MESSAGE);
+    let handler = move |_: HyperRequest, response: HyperResponse| {
+        response.send(MESSAGE).unwrap();
+    };
+    let (mut server, url) = make_server(handler);
 
     let mut request = Request::new(url, Context::Fetch, false);
     request.referer = Referer::NoReferer;
@@ -47,7 +51,10 @@ fn test_fetch_response_is_not_network_error() {
 fn test_fetch_response_body_matches_const_message() {
 
     static MESSAGE: &'static [u8] = b"Hello World!";
-    let (mut server, url) = make_server(MESSAGE);
+    let handler = move |_: HyperRequest, response: HyperResponse| {
+        response.send(MESSAGE).unwrap();
+    };
+    let (mut server, url) = make_server(handler);
 
     let mut request = Request::new(url, Context::Fetch, false);
     request.referer = Referer::NoReferer;
@@ -60,6 +67,72 @@ fn test_fetch_response_body_matches_const_message() {
         ResponseBody::Done(body) => {
             assert_eq!(body, MESSAGE);
         },
-        _ => { panic!() }
+        _ => panic!()
+    };
+}
+
+fn test_fetch_redirect_count(message: &'static [u8], redirect_cap: u32) -> Response {
+
+    let handler = move |request: HyperRequest, mut response: HyperResponse| {
+
+        let redirects = match request.uri {
+            RequestUri::AbsolutePath(url) =>
+                url.split("/").collect::<String>().parse::<u32>().unwrap_or(0),
+            RequestUri::AbsoluteUri(url) =>
+                url.path().unwrap().last().unwrap().split("/").collect::<String>().parse::<u32>().unwrap_or(0),
+            _ => panic!()
+        };
+
+        if redirects >= redirect_cap {
+            response.send(message).unwrap();
+        } else {
+            *response.status_mut() = StatusCode::Found;
+            let url = format!("{redirects}", redirects = redirects + 1);
+            response.headers_mut().set(Location(url.to_owned()));
+        }
+    };
+
+    let (mut server, url) = make_server(handler);
+
+    let mut request = Request::new(url, Context::Fetch, false);
+    request.referer = Referer::NoReferer;
+    let wrapped_request = Rc::new(request);
+
+    let fetch_response = fetch(wrapped_request, false);
+    let _ = server.close();
+    fetch_response
+}
+
+#[test]
+fn test_fetch_redirect_count_ceiling() {
+
+    static MESSAGE: &'static [u8] = b"no more redirects";
+    // how many redirects to cause
+    let redirect_cap = 20;
+
+    let fetch_response = test_fetch_redirect_count(MESSAGE, redirect_cap);
+
+    assert_eq!(Response::is_network_error(&fetch_response), false);
+    match fetch_response.body {
+        ResponseBody::Done(body) => {
+            assert_eq!(body, MESSAGE);
+        },
+        _ => panic!()
+    };
+}
+
+#[test]
+fn test_fetch_redirect_count_failure() {
+
+    static MESSAGE: &'static [u8] = b"this message shouldn't be reachable";
+    // how many redirects to cause
+    let redirect_cap = 21;
+
+    let fetch_response = test_fetch_redirect_count(MESSAGE, redirect_cap);
+
+    assert_eq!(Response::is_network_error(&fetch_response), true);
+    match fetch_response.body {
+        ResponseBody::Done(_) => panic!(),
+        _ => { }
     };
 }