Api server test for dispatch operator

[ACEmilG]

This is a fresh attempt at testing the API server. See PR #123 for previous try.

[igorpeshansky]

Hmm... Do you have to put the whole test into a static function in the fixture? Would it make more sense to add functions to create and invoke the dispatcher instead? Also below.

[ACEmilG]

The problem is that HttpServer, Handler, HandlerMap, and even Dispatcher, are all declared private. We could attempt to pull out small portions of the test, but that seems confusing and difficult without any gain.

[igorpeshansky]

It should be enough to expose a Dispatcher test factory and an invoker, e.g.:

class ApiServerTest : public ::testing::Test {
 protected:
  using Dispatcher = MetadataApiServer::Dispatcher;
  std::unique_ptr<Dispatcher> CreateDispatcher(
      const std::map<std::pair<std::string, std::string>,
                     std::function<void()>>& handlers) {
    Dispatcher::HandlerMap handler_map;
    for (const auto& element : handlers) {
      std::function<void()> handler = element.second;
      handler_map.emplace(element.first, [handler](
          const MetadataApiServer::HttpServer::request& request,
          std::shared_ptr<MetadataApiServer::HttpServer::connection> conn) {
        handler();
      });
    }
    return std::unique_ptr<Dispatcher>(new Dispatcher(handler_map, false));
  }
  void InvokeDispatcher(
      const std:: unique_ptr<Dispatcher>& dispatcher,
      const std::string& method, const std::string& path) {
    MetadataApiServer::HttpServer::request request;
    request.method = method;
    request.destination = path;
    (*dispatcher)(request, nullptr);
  }

Then you can use the following in the test, e.g.:

  bool handler_called;
  std:: unique_ptr<Dispatcher> dispatcher = CreateDispatcher({
    {{"GET", "/testPath/"}, [&handler_called]() {
      handler_called = true;
    }},
  });
  InvokeDispatcher(dispatcher, "GET", "/testPathFoo/");
  EXPECT_FALSE(handler_called);
  InvokeDispatcher(dispatcher, "GET", "/test/");
  EXPECT_FALSE(handler_called);
  InvokeDispatcher(dispatcher, "GET", "/testFooPath/");
  EXPECT_FALSE(handler_called);

[ACEmilG]

Very cool, changed, much better.

[igorpeshansky]

Why is this needed? Isn't GetMetadataMap public?

[ACEmilG]

Backed out these changes - sorry about that, I should have remembered before pushing.

[igorpeshansky]

It should be enough to expose a Dispatcher test factory and an invoker, e.g.:

class ApiServerTest : public ::testing::Test {
 protected:
  using Dispatcher = MetadataApiServer::Dispatcher;
  std::unique_ptr<Dispatcher> CreateDispatcher(
      const std::map<std::pair<std::string, std::string>,
                     std::function<void()>>& handlers) {
    Dispatcher::HandlerMap handler_map;
    for (const auto& element : handlers) {
      std::function<void()> handler = element.second;
      handler_map.emplace(element.first, [handler](
          const MetadataApiServer::HttpServer::request& request,
          std::shared_ptr<MetadataApiServer::HttpServer::connection> conn) {
        handler();
      });
    }
    return std::unique_ptr<Dispatcher>(new Dispatcher(handler_map, false));
  }
  void InvokeDispatcher(
      const std:: unique_ptr<Dispatcher>& dispatcher,
      const std::string& method, const std::string& path) {
    MetadataApiServer::HttpServer::request request;
    request.method = method;
    request.destination = path;
    (*dispatcher)(request, nullptr);
  }

Then you can use the following in the test, e.g.:

  bool handler_called;
  std:: unique_ptr<Dispatcher> dispatcher = CreateDispatcher({
    {{"GET", "/testPath/"}, [&handler_called]() {
      handler_called = true;
    }},
  });
  InvokeDispatcher(dispatcher, "GET", "/testPathFoo/");
  EXPECT_FALSE(handler_called);
  InvokeDispatcher(dispatcher, "GET", "/test/");
  EXPECT_FALSE(handler_called);
  InvokeDispatcher(dispatcher, "GET", "/testFooPath/");
  EXPECT_FALSE(handler_called);

[igorpeshansky]

Did you forget to initialize this? Also below.

[ACEmilG]

Done

[igorpeshansky]

There is actually a bug in line 43 of api_server.cc — reverse iterators should still be incremented with ++. Can you please change --it to ++it?

[ACEmilG]

fixed

[igorpeshansky]

Another thing we can clean up here is making both operator() and log() const in Dispatcher. Can you please do this?

[ACEmilG]

Done

[igorpeshansky]

This is looking better. Some more comments.

[igorpeshansky]

Sigh, I thought I've caught them all... That's what I get for typing in code on my phone.
Let's remove the space after the ::.

[ACEmilG]

Sorry - I should have caught this myself

[igorpeshansky]

How about better test names? I'd call this one DispatcherMatchesFullPath.

[ACEmilG]

Sure, changed

[igorpeshansky]

There's not enough signal here (e.g., imagine if the implementation accidentally called every handler — if this one got called first, you'd never know). How about adding a bad_handler_called boolean for this one (and setting it to true in the handler)?

[ACEmilG]

Makes sense, added.

[igorpeshansky]

How about DispatcherUnmatchedMethod?

[ACEmilG]

Sure, changed

[igorpeshansky]

Should this also check that an actual substring does match (e.g., /testPath/subPath/)?

[ACEmilG]

Good idea, added

[igorpeshansky]

LGTM

Does this need rebasing against master?

[supriyagarg]

A rebase against master is required, but otherwise LGTM.

[ACEmilG]

Rebased

[igorpeshansky]

Should this have been "/testPath/subPath/" (note the trailing /)?

[ACEmilG]

Fixed

[igorpeshansky]

LGTM