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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
SteffenStautmeister opened this issue Jul 26, 2023 · 1 comment

Comments

@SteffenStautmeister
Copy link

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
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants