lipanski / mockito

HTTP mocking for Rust!
MIT License
683 stars 59 forks source link

Consider exposing server #92

Closed ggriffiniii closed 1 year ago

ggriffiniii commented 4 years ago

Hi, I just discovered mockito and it looks pretty useful. One question I had when reading the examples and docs and having a brief look at the implementation. Have you considered exposing the server and letting users control when a server is started? The global nature of the server was the first thing that stood out to me as a quirk when using the library. The fact that all my tests were no longer parallel and there was no way for me to control it was a drawback. I looked through old closed issues to see if this had been asked before but didn't find anything so thought I would ask.

Concretely. Where my tests currently look like:

#[test]
fn my_test() {
    let m = mock("GET", "/hello")
      .with_status(201)
      .with_header("content-type", "text/plain")
      .with_header("x-api-key", "1234")
      .with_body("world")
      .create();

    // Issue my requests to mockito::server_url()

    m.assert();
}

I would like to be able to write something like this:

#[test]
fn my_test() {
    let server = mockito::server::run();
    let m = server.mock("GET", "/hello")
      .with_status(201)
      .with_header("content-type", "text/plain")
      .with_header("x-api-key", "1234")
      .with_body("world")
      .create();

    // Issue my requests to server.url()
}

This is personal preference, but I much prefer the latter API. I could easily be overlooking something making it difficult to implement in that way, but to me it reads much more clearly and provides additional flexibility (easy to start one server per test if desired, sharing a server between all tests would also be easy by putting it into a OnceCell similar to what the implementation does currently).

If you're open to this idea I could look into writing a PR.

lipanski commented 4 years ago

@ggriffiniii interesting idea. how would expose the server address to your code though (in the least intrusive way)? talking about this. maybe thread-local variables could be an option but they would only work in the main thread.

ggriffiniii commented 4 years ago

I was thinking server would just have methods that return the address and URL. They wouldn't need to be global or have any synchronization since code under test would have exclusive ownership of the server variable (unless it chooses to share it obviously).

lipanski commented 4 years ago

@ggriffiniii in this case, you'd need to expose a function that sets your remote URL, correct? is there any other way - maybe with the use of compiler flags? I'm not sure how common it is to expose the remote URL as function. it's not the end of the world but generally I wouldn't want to introduce support code only for the purpose of testing. could you provide a more detailed example (one that contains how you'd pass the URL)?

ggriffiniii commented 4 years ago

@lipanski I'm not quite sure I understand correctly. It sounds like you're asking how I would write my code to be tested, such that it could make requests to the dynamic url of the test server. Am I understanding that correctly?

If so, I don't see how it's that different from using mockito::server_url() today. Here is an example (this is not my code, just the first example on github for a search of 'language:rust mockito').

This is the code that exists today here:

#[tokio::test]
async fn accept_ranges_none() {
    let url = &mockito::server_url();

    let logger = NullLoggerBuilder.build().unwrap();

    let _head_mock = mockito::mock("HEAD", "/")
        .with_status(200)
        .with_header("content-length", "10")
        .with_header("accept-ranges", "none")
        .create();

    let options = FetchOptions {
        url: url.to_owned(),
        output_option: None,
        num_fetches: 1,
        logger: logger.clone(),
        check_etag: false,
        max_retries: 1,
    };

    let result = fetch(options).await;
    debug!(logger, "fetch finished"; "result" => format!("{:?}", &result));

    let error = result.expect_err("testing");

    if let FetchError::ServerSupportError(msg) = *error {
        assert_eq!("Server's Accept-Ranges header set to none", msg);
    } else {
        panic!("Expected ServerSupportError");
    }
}

I would like to be able to change this code to:

#[tokio::test]
async fn accept_ranges_none() {
    let server = mockito::sever::run();

    let logger = NullLoggerBuilder.build().unwrap();

    let _head_mock = server.mock("HEAD", "/")
        .with_status(200)
        .with_header("content-length", "10")
        .with_header("accept-ranges", "none")
        .create();

    let options = FetchOptions {
        url: server.url(),
        output_option: None,
        num_fetches: 1,
        logger: logger.clone(),
        check_etag: false,
        max_retries: 1,
    };

    let result = fetch(options).await;
    debug!(logger, "fetch finished"; "result" => format!("{:?}", &result));

    let error = result.expect_err("testing");

    if let FetchError::ServerSupportError(msg) = *error {
        assert_eq!("Server's Accept-Ranges header set to none", msg);
    } else {
        panic!("Expected ServerSupportError");
    }
}

Does this demonstrate how the address could be used? This is just the first example on github, there are many that behave similarly. I apologize if I misunderstood the question.

lipanski commented 4 years ago

if you browse a bit through the list of Rust projects on Github using Mockito you can see two types of projects:

(1) Projects that use dependency injection to expose the base URL and hook it up to mockito::server_url() during tests.

Some examples:

(2) Projects that use compiler flags to hook up mockito::server_url() during tests.

Some examples:


my concerns are regarding the projects that use compiler flags (the second group). those projects would have to introduce support methods for setting the base URL, which comes with some overhead and generally doesn't fit all projects.

@ggriffiniii how would you handle those cases? I guess compiler flags would be completely out of the question and those projects would have to update to dependency injection if they'd like to upgrade Mockito.

worst case we could have backwards compatibility, but not sure it's worth it. I was actually quite surprised that the majority of projects prefer dependency injection over compiler flags.

I'd be tempted to go with your suggestion.

@lucab @thomasetter @kornelski @loyd any opinion on this?

lucab commented 4 years ago

@lipanski I maintain zincati in the (1) list and afterburn in the (2) list. I think I can easily switch the latter to inject the server base URL. I would say it also feels better as it makes resource ownership local, so I'm overall +1 on this.

A couple of questions:

ggriffiniii commented 4 years ago

Yes, my solution would not allow conditional compilation to override the server address. I've honestly never had that to be an option for an app I've written as the url almost always comes from some type of runtime configuration.

I started writing my own implementation, initially just to solidify how this could be done, but I ended up liking the results quite a bit and just published it on httptest. It's more of a trait based approach to matching and responding so I like the flexibility and explicitness it provides. I also demonstrate how the zincati tests would look using httptest, just as a validation of my approach using a real world test case.

Regarding port exhaustion: It would be pretty trivial to add a server pool to limit the number of servers running.

lipanski commented 4 years ago

@ggriffiniii sounds good. are you still interested in porting those changes to Mockito? if so, I'd be happy to accept such a PR.

I'd also be curious how we could implement the server pool idea, mainly how you'd make sure that tests running in parallel test threads never end up using the same server instance.

ggriffiniii commented 4 years ago

At this point I'm pretty happy with httptest so my motivation for submitting a PR has been nullified. If you are still interested in making this change to mockito or if you want to take any other parts from httptest and pull it into mockito I would be happy to help.

Regarding how the server pool idea could be implemented have a look at httptest::ServerPool. It implements a pool of servers that can be const initialized and shared between tests. A httptest::ServerPool::new(1) is effectively the way that mockito operates today.

lipanski commented 1 year ago

Fixed in https://github.com/lipanski/mockito/releases/tag/0.32.0

do-nat commented 1 year ago

@lipanski is it still possible to somehow use mockito to define the base URL with compiler flags?