Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal to remove lifetime from TestResponse #75

Merged
merged 2 commits into from Dec 21, 2017

Conversation

bradleybeddoes
Copy link
Contributor

In 3rd party code it may be useful to setup a TestServer and TestResponse within a common function, returning both to the the test harness for further evaluation.

This is certainly a pattern I use a lot within my tests, that was "broken" by the recently merged changes to Gotham testing.

Example that is possible if this PR is merged that currently isn't on master:

fn send_request(uri: String, accept: Option<mime::Mime>) -> (TestServer<Router>, TestResponse) {
    env::set_var("APP_ENV", "test");
    let mut test_server = TestServer::new(router()).unwrap();
    let mut req = Request::new(Method::Get, uri.parse().unwrap());
    match accept {
        Some(accept) => req.headers_mut().set(Accept(vec![qitem(accept)])),
        None => (),
    };
    let res = test_server
        .client()
        .perform(req)
        .unwrap();

    (test_server, res)
}

#[test]
fn valid_abc() {
    let subject = format!("acct:{}", fake!(Internet.free_email));
    let (_test_server, res) = send_request(web_finger_uri(&subject, OIDC_DISCOVERY_REL), None);
    
    // asserts
}

#[test]
fn valid_xyz() {
    let subject = format!("acct:{}", fake!(Internet.free_email));
    let (_test_server, res) = send_request(web_finger_uri(&subject, OIDC_DISCOVERY_REL), Some(custom_mime_type());
    
   // asserts
}

This hasn't been discussed, prior to my landing of this PR, as being a good idea or not so lets do that now 馃榿.

In 3rd party code it may be useful to setup a TestServer and TestResponse
within a common method, returning both to the the test harness for
further evaluation.

With lifetimes as specified this was a lot harder to achieve and meant
tests violated DRY to make this happen.

These changes specifically address this use case via an Rc, thus
eliminating the need for a lifetime specifier.
@coveralls
Copy link

coveralls commented Dec 20, 2017

Coverage Status

Coverage increased (+0.06%) to 81.237% when pulling a01154a on refine-test-helpers into c18ad43 on master.

@smangelsdorf smangelsdorf added this to the 0.2 milestone Dec 20, 2017
@smangelsdorf
Copy link
Contributor

Sounds like a good idea. I'll save the proper review for a time of day where I'm more alert 馃榿

Previous implemention was unnecessarily being caught up in the public
API.
@coveralls
Copy link

coveralls commented Dec 20, 2017

Coverage Status

Coverage increased (+6.7%) to 87.893% when pulling 21111ff on refine-test-helpers into c18ad43 on master.

@smangelsdorf smangelsdorf merged commit 953c39b into master Dec 21, 2017
@smangelsdorf smangelsdorf deleted the refine-test-helpers branch December 21, 2017 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants