Migrate KitchenSink examples into integration tests

bradleybeddoes commented 7 years ago

KitchenSink is a great example of the various pieces of Gotham functioning together but is has grown to the point where maintaining it manually is no longer feasible.

We'll get much better value by splitting this up into integration tests that can be run through CI whilst still maintaining its original purpose as a set of basic examples for working with each component of Gotham.

bradleybeddoes commented 7 years ago

Incorrectly closed during source control system migration.

chklauser commented 6 years ago

Hi,

I decided to have a go at #14, mostly as an exercise to get to know gotham a bit better. My development branch is over here: migrate-kitchen-14:./tests (diff).

Before I continue, I would like to know if this is broadly going into the direction you had imagined and maybe what your opinion is on some of my decisions.

Splitting up `kitchen-sink`

As its name suggests, the kitchen-sink example demonstrates a number of different features together in one application. I intend to split it up into 3 separate test crates:

echo_server.rs Just static routes. Establishes a base level for the setup code that gotham needs.
dynamic_xxx.rs (TODO) dynamic routes, path extractors and query extractors
middleware_server.rs Just the middleware with some handler that interacts with it

Each test create can run multiple tests against the test server it sets up.

Would you prefer a different split?

`TestServer` vs. `gotham::start`

The kitchen-sink example was a full-blown example application, running multiple listener-threads and spawning a handler thread for each request. Initially I had a look at the TestServer struct, but it seems more targeted towards unit testing (single threaded).

I decided to stick closer to kitchen-sink and wrote a small function that lets multiple tests (run concurrently by cargo) share a single server implementation. One of the tests will be the first to secure a Mutex and gets to start a the server. The other tests wait until the server accepts TCP connections.

Seemed to work reliably enough, but I'm not sure if that's what you guys are looking for. Technically, all of the interesting stuff happens in the build_router() function and unless we start spawning threads in the individual test functions, there is probably no between TestServer and "the real thing". Personally, I think that the integration tests would be the place to use "real" implementations where possible.

(One TODO in my implementation: need to ensure the build_router() function only runs once.)

Which approach seems more useful to you?

HTTP Client Library

One downside of not using TestServer is that the individual test functions don't have access to an already running tokio Core. I didn't want to fiddle around with plain Hyper at that level, so I pulled in reqwest as a dev-dependency. Works perfectly fine for testing, but I don't like that reqwest is pulling in a mandatory dependency on OpenSSL/rust-native-tls.

I had a look around for other client libraries, but none of the others seem as well-maintained as reqwest. Unless we can convince the reqwest maintainers to make TLS-support an optional feature, I'd tend towards not pulling it in as a dependency and rewriting my test cases in Hyper.

What do you think?

Test Architecture

Code that is shared between all tests goes into support/mod.rs (or one of the modules that it includes). Each test crate has a mod support; statement to pull in that code. It includes logging and the shared server setup mentioned above. I would like to put as little code as possible into this module hierarchy, because it is re-compiled for each test crate.

Code that is shared by some but not all test crates goes into parts/xxx.rs. The *.rs files in that directory are not part of a mod-structure because each test crate needs to decide which of the modules in there it wants to use. As a result, the test crates need to specify the path explicitly.

#[path = "parts/echo.rs"]
mod echo

One alternative would be to create separate directories with mod.rs files (echo/mod.rs, middleware/mod.rs), but depending on your editor, this might be a sub-optimal experience. Another would be to include! the files as rust source text, but I really dislike that approach because it confuses IDEs/editors even more.

Opinions? Preference? Ideas?

Other

I assume the final pull request should include the deletion of kitchen-sink, right?

Other than that, is there anything I have overlooked?

smangelsdorf commented 6 years ago

Thanks for working on this. I'll respond to each of your points under the same headings you raised them:

Splitting up kitchen-sink

Sounds like a great starting point. :+1:

`TestServer` vs. `gotham::start`

I certainly understand what you're saying about TestServer currently looking as though it's geared towards unit tests rather than integration tests. There's a lot to be said for keeping everything as close to production as possible during an integration test.

That said, there's very little that distinguishes gotham::start_with_num_threads (threads=1) from a TestServer app (it's mostly stuff at the Tokio/Hyper level). My preference would be to evolve TestServer in a direction that makes it suitable for integration testing as well.

I have an active branch at the moment where I'm working on #16, so I'll give some thought to how I could use more of the "production" code there. Happy to take any thoughts.

HTTP Client Library

If there's a legitimate need to spin up Gotham apps in some kind of integration test harness, I'm sure we'll eventually provide a helper which gives both a running Gotham app and a Tokio Core on which to run requests.

I suspect such an API would end up being very similar to TestServer, though, and the best approach here might be to use TestServer with the understanding that it'll evolve to be the "right way" to do integration tests.

Test Architecture

I like this structure, and I think the benefits outweigh the downside of having to recompile the support code for each integration test.

I agree that including the rust source as text would be worse (and wouldn't negate that downside either).

The only thing we lose here is dead code detection. I suspect there isn't much we could do about that, short of having another crate just for support code (which introduces a circular dependency of gotham -> support code -> gotham).

Other

The goal here is to remove the kitchen-sink project in favour of something more maintainable, so removing it seems like a good idea.

chklauser commented 6 years ago

Cool, thanks for the feedback 😄. In that case, I'll finish the migration and use TestServer to implement the tests.

We could move the integration tests into a dedicated testing package that depends on gotham, say gotham-test. We'd have to use Cargo workspaces to ensure that the two packages use the same set of dependencies.

The big downside of that approach is that such integration tests would no longer be part of gotham. One would have to run them explicitly in a separate cargo invocation. Not a problem for travis but I'm sure people will forget to run those separate tests.

A handful of dead_code warnings in test code are probably the lesser evil. I'd instead revisit the "separate integration test package"-idea if compilation times for the integration tests start to get annoying.

smangelsdorf commented 6 years ago

I have an active branch at the moment where I'm working on #16, so I'll give some thought to how I could use more of the "production" code there. Happy to take any thoughts.

This is now available in #52, if you'd like to see it and give any feedback.

Hopefully the change in API makes it feel more like gotham::start, though not much has changed internally. The fact that it uses NewHandlerService at the top level means the only real difference is the way connections are established before being given to Hyper.

chklauser commented 6 years ago

I finished the migration using the "real", shared server instances: chklauser/gotham:migrate-kitchen-14

I also did a port using the testing-api branch over at chklauser/gotham:experimental-kitchen. The change was surprisingly pleasant. It helps that both client APIs re-use much of Hyper. Some thoughts:

a read_utf8_body() method would be nice. Otherwise, I imagine many people will keep writing let buf = String::from_utf8(res.read_body().expect("readable response")).expect("UTF8 response"); over and over again.
if there is no "fluent" interface for configuring requests, there is probably not much point in having .get(...), .post(...), .put(...) etc. methods. For gotham tests, these might suffice, but I imagine that real applications will almost always have to supply additional headers. While I think it would be great to have cool, fluent builders for requests, a first version of the API is perfectly functional without those. I would even remove .get().
Switching between "asserting headers" and "asserting the body" (consumes response) is awkward. I understand why it is this way, but below you can see an example of the gymnastics I have to perform to talk about a header value and the body in a single statement. (Should all be better with NLL)
Is it really correct to be able to look at the headers before the body has been evaluated? Since we are only running a single thread, couldn't some header manipulations be hidden inside the "body future"?

So far so good, but one of my test cases fails after the switch to the new gotham::test::TestServer. In the echo-test with sending a POST body, I get a very weird response:

#[test]
fn post_echo() {
    let server = test_server(build_router());
    let client = server.client();

    let data = "This text should get reflected back to us. \
                      Even this fancy piece of unicode: \u{3044}\u{308d}\u{306f}\u{306b}\u{307b}";
    let mut req = Request::new(Method::Head,
                               Uri::from_str("http://host/echo").expect("uri"));
    req.set_body(data);
    req.headers_mut().set(ContentType(mime::TEXT_PLAIN));
    let res = client.request(req).expect("request successful");
    assert_eq!(res.status(), StatusCode::Ok);
    let content_length = {
        let content_type: Option<&ContentType> = res.headers().get();
        assert!(content_type.is_some(), "Content-Type header missing from response");
        assert_eq!(content_type.unwrap().0, mime::TEXT_PLAIN);

        let content_length: Option<&ContentLength> = res.headers().get();
        assert!(content_length.is_some(), "{} header missing from response", ContentLength::header_name());
        content_length.unwrap().0
    };
    let buf = String::from_utf8(res.read_body().expect("readable response")).expect("UTF8 response");
    assert_eq!(content_length, data.as_bytes().len() as u64, "Response body: {:?}", buf);
    assert_eq!(buf, data);
}

// the handler
pub fn post(mut state: State) -> Box<HandlerFuture> {
    trace!("Echo::post");
    let f = Body::take_from(&mut state).concat2().then(
        move |full_body| {
            match full_body {
                Ok(valid_body) => {
                    let resp_data = valid_body.to_vec();
                    debug!("echo: {:?}", resp_data);
                    let res = create_response(
                        &state,
                        StatusCode::Ok,
                        Some((resp_data, mime::TEXT_PLAIN)),
                    );
                    future::ok((state, res))
                }
                Err(e) => future::err((state, e.into_handler_error())),
            }
        },
    );

    Box::new(f)
}

The Content-Length comparison and the response body comparison fail:

2017-09-29 21:47:15.365744594 1WvsQ5xRyTp echo_server::support::server TRACE: start_server begin
2017-09-29 21:47:15.366062964 1WvsQ5xRyTp echo_server::support::server TRACE: start_server end
2017-09-29 21:47:15.370093320 1WvsQ5xRyTp echo_server::echo TRACE: Echo::get
thread 'post_echo' panicked at 'assertion failed: `(left == right)`
  left: `14`,
 right: `92`: Response body: ""', tests/echo_server.rs:183:4
stack backtrace:

Observations:

We expect 92 bytes, server claims Content-Length: 14
Server actually delivers zero bytes
The debug! message in the response handler isn't displayed (Future not fully evaluated?)

Am I doing something wrong here or is that a bug?

smangelsdorf commented 6 years ago

Certainly seems like a bug, and one I'll definitely look into further before #52 gets merged. Thanks for the detailed write-up here, it gives me a lot to work with. Even if it's technically not a bug (though I suspect it is), it shouldn't be so easy to get incorrect behaviour like this.

a read_utf8_body() method would be nice. Otherwise, I imagine many people will keep writing let buf = String::from_utf8(res.read_body().expect("readable response")).expect("UTF8 response"); over and over again.

Good idea.

if there is no "fluent" interface for configuring requests, there is probably not much point in having .get(...), .post(...), .put(...) etc. methods. For gotham tests, these might suffice, but I imagine that real applications will almost always have to supply additional headers. While I think it would be great to have cool, fluent builders for requests, a first version of the API is perfectly functional without those. I would even remove .get().

You're right. Maybe there's something simple I can do here to improve it a bit. I'll have a look.

Switching between "asserting headers" and "asserting the body" (consumes response) is awkward. I understand why it is this way, but below you can see an example of the gymnastics I have to perform to talk about a header value and the body in a single statement. (Should all be better with NLL)

Is it really correct to be able to look at the headers before the body has been evaluated? Since we are only running a single thread, couldn't some header manipulations be hidden inside the "body future"?

Good points here. Last time I looked, this seemed like a limitation of the Hyper client, and looking again it seems not to have changed (unless I'm missing something). We'll need proper chunked response support in Hyper's client before we can support it.

smangelsdorf commented 6 years ago

@chklauser: Apologies for the delay in getting to this. I've been snowed under with other work.

I've investigated your test case and tried to reproduce it in my own test case, and I was able to fix it by changing:

    let mut req = Request::new(Method::Head,
                               Uri::from_str("http://host/echo").expect("uri"));

to:

    let mut req = Request::new(Method::Post,
                               Uri::from_str("http://host/echo").expect("uri"));

That is, it seems this issue is being triggered by the HTTP method not being correct for the kind of request that's being made. Despite what the handler is doing, it seems that Method::Head is triggering (presumably correct or at least intended) behaviour elsewhere in the stack where it will neither accept a request body, nor deliver a response body.

Keeping this behaviour in mind, I'm unable to trigger the other problem you saw with the content length. Rather, I always see a content length of 0. If you're still able to trigger this after updating Hyper, I'd be interested to dig further into this. I'm on Hyper 0.11.6.

I can confirm that the debug!(_) message doesn't display for me, but if I change to a println!(_) it does (albeit only on failure, as is typical for tests). I expect that initializing logging for test cases would fix this.

I've added a new test case to my branch to ensure this case continues to work correctly, and I no longer consider #52 to be blocked on this problem. At worst, this doesn't appear to be a regression from the TestServer we've already shipped, and in any case seems not to be a bug in Gotham itself.

bradleybeddoes commented 6 years ago

Hey @chklauser just thought I'd check in with you on this issue, see how you're progressing etc.

Have you seen https://github.com/gotham-rs/gotham/pull/70 which may have some impact here?

n.b we've also merged https://github.com/gotham-rs/gotham/pull/52 and https://github.com/gotham-rs/gotham/pull/75 which could be helpful here.

bradleybeddoes commented 6 years ago

Thanks for everyone's hard work on this issue.

With the release of Gotham 0.2 we're basically done with this now, essentially all of the code is now present in https://github.com/gotham-rs/gotham/tree/master/examples with great PR in progress for the remainder.

gotham-rs / gotham