pistacheio / pistache

A high-performance REST toolkit written in C++
https://pistacheio.github.io/pistache/
Apache License 2.0
3.12k stars 688 forks source link

Unit testing of individual routes/path - Too many responsibilities of Pistache::Http::ResponseWriter #1150

Open SteffenStautmeister opened 11 months ago

SteffenStautmeister commented 11 months ago

Hi,

we use pistache as webserver to provide a RESTApi which is generated from a openAPI specification. We want to test the individual routes with a unit test framework (doctest) and see some problems with the Pistache::Http::ResponseWriter. In our opinion, the Pistache::Http::ResponseWriter has too many dependencies, tasks and responsibilities. Currently Pistache::Http::ResponseWriter is responsible for the building/writing and sending of the responses and is dependent on Pistache::Http::Handler, Pistache::Tcp::Transport and Pistache::Tcp::Peer. That makes the testing from the individual route very difficult.

For example, a simple route that we want to test:

void UserManagementApi::check_user_id_get(const std::int32_t id, Pistache::Http::ResponseWriter &response) 
{
  if(id == 1) {
    response.send(Pistache::Http::Code::Ok, "user id valid\n");
  }
  else {
    response.send(Pistache::Http::Code::Bad_Request, "User id not valid\n");
  }
}

The unit test for this route looks like the following code:

#include "doctest.h"
#include "UserManagementApi.h"

#include <memory>
#include <optional>
#include <pistache/http.h>
#include <pistache/router.h>

class DummyTCPHandler : public Pistache::Http::Handler {
  public:
  void onInput(const char* buffer, size_t len, const std::shared_ptr<Pistache::Tcp::Peer>& peer) override {
  }
  void onRequest(const Pistache::Http::Request& request, Pistache::Http::ResponseWriter response) override {
  }
  std::shared_ptr<Pistache::Tcp::Handler> clone() const override {
    return std::shared_ptr<Pistache::Tcp::Handler>();
  }
};

TEST_SUITE("test REST - UserManagementApi") {
  TEST_CASE("check user id "){
    Pistache::Rest::Router router;
    UserManagementApi userManagementApi(router);

    std::shared_ptr<DummyTCPHandler> tcpHandler = std::make_shared<DummyTCPHandler>();
    Pistache::Tcp::Transport transport(tcpHandler);
    std::weak_ptr<Pistache::Tcp::Peer> peer;

    Pistache::Http::ResponseWriter responseWriter(Pistache::Http::Version::Http10, &transport, tcpHandler.get(), peer);

    userManagementApi.check_user_id_get(1, responseWriter);

    CHECK(responseWriter.getResponseCode() == Pistache::Http::Code::Ok);
  }
}

To test the individual route we need a lot of dependencies and dummy objects to create a Pistache::Http::ResponseWriter object, just only to test the business logic of the individual routes. In our opinion, it is better if the responsibilities are separated in e.g. when building/writing and sending responses. Then the business logic can be tested more easily and the dependencies are more clearly defined.

Are there similar considerations in the pistache project? Does anyone see the same issues or have we missed something. We are very interested in your opinion and what you think about it.

Thanks in advance.

Tachi107 commented 11 months ago

Hi @SteffenStautmeister, thanks for your feedback!

To test the individual route we need a lot of dependencies and dummy objects to create a Pistache::Http::ResponseWriter object, just only to test the business logic of the individual routes. In our opinion, it is better if the responsibilities are separated in e.g. when building/writing and sending responses. Then the business logic can be tested more easily and the dependencies are more clearly defined.

I kinda understand your point. When I was using Pistache for a (small) project, what I did was simply spin up a test server on localhost, send HTTP requests to it, and inspect the results. You can see a simple example here: https://github.com/PwRAu/scraf-backend/blob/9edcd323b8d174aaf6ce29fd7f5a4456c56624f0/tests/students.cpp#L31C1-L52. What the SCRAF_TEST_SERVER() macro does is spin up a temporary server which handles requests for that specific test. The server depends on an SQL server, but that can be replaced at compile time via dependency injection - the fake database is defined in tests/common.hpp, while the dependency-injectable server is defined in scraf.hpp.

I wrote the code I linked above a couple of years ago, when I was in high school, so it is very possible that my methodology was flawed and a simpler solution existed. But it worked well for us at the time.

I also have limited knowledge of other REST frameworks and how they approach unit testing (since what I did above arguably isn't unit testing, but something more like integration testing). What do you have in mind with a more modular ResponseWriter?

Unrelated: is there a reason why you are using HTTP 1.0 instead of HTTP 1.1?