Increase the test coverage by adding HttpTests

[johannesduesing]

There are already tests in place that test the business logic (RequestHandler) and data access (InstanceDAO). We want to increase the test coverage of the overall project by adding HttpTests using the akka-http-testkit. These tests should verify that

• Correct error codes are returned for wrong parameter syntax (missing, misspelled, etc)
• Correct error codes are returned for semantically wrong requests (invalid id -> 404, invalid instance format -> 400, etc)
• Correct data is returned for valid requests (register -> get id, getInstanceOfType -> instance, etc..)

[Ayybeeshafi]

@johannesduesing can you please review whenever you find some time. Let me know if the test coverage is enough or I am missing anything.

[johannesduesing]

@Ayybeeshafi thanks for your submission :+1: Next time please open a PR if you think your work is done, its way easier to give feedback in PRs. As for your test, i checked it out and found quite a lot of problems:

The tests are not actually being executed In fact, the code in the braces after the check is never being executed. This is a snippet from your test class

 "Successfully Registered" in {
      Post("register") -> server.Route -> check {
        assert(status === StatusCodes.OK)
        assert(response.status === 200)
        val registerNewInstance = handler.handleRegister(buildInstance(Long.MaxValue))
        assert(registerNewInstance.isSuccess)
      }
    }

Since you haven't started a server, your tests do not connect to anything. The server.Route you are addressing in your code is imported from import akka.http.scaladsl.server and has nothing to do with our application. I was able to make it work by starting the registry first

override def beforeAll(): Unit = {
    Future(Registry.main(Array[String]()))
    Thread.sleep(3000)
  }

  override def afterAll(): Unit = {
    System.exit(0)
  }

and then changing the test to use the correct routes from our server implementation at import de.upb.cs.swt.delphi.instanceregistry.connection.Server.routes, which in turn allows me to change the test to

"Successfully Registered" in {
      Post("/register") ~> routes ~> check {
        assert(status === StatusCodes.OK)
        assert(response.status === 200)
        val registerNewInstance = handler.handleRegister(buildInstance(Long.MaxValue))
        assert(registerNewInstance.isSuccess)
      }
    }

Notice you have to use the operator ~> instead of ->, and you have to insert a slash before the path. Please note: I am not sure if this is the default way of using the akka-http-testkit, maybe there is an easier way without starting the whole application in the background. That research is part of your work though.

The test is not testing the http implementation Your test from the snippet above shows that you are directly accessing the Business Logic of our application, namely the RequestHandler

 val registerNewInstance = handler.handleRegister(buildInstance(Long.MaxValue))
assert(registerNewInstance.isSuccess)

The RequestHandler already is tested with its own test file. You task is to test the API logic, which is in the Server.scala. The whole point of the http tests is to see wether the implementation of the Server is correct, since we already know that the RequestHandler is working as inteded. This is why you should only be calling HTTP endpoints, and never directly access the RequestHandler.

There is no semantic testing As discussed last time, it would make sense to test scenarios like the following:

1. Add an instance by calling Post("/register") with the correct JSON serializied request body.
2. Assert the status is 200 OK and the server returned an id, e.g. 42.
3. Call GET("/instance?Id=42")
4. Assert the status is 200 OK and the server returned an instance with the same attributes as the one registered in 1.

Currently, none of these scenarios are tested.

No parameters are being passed to the endpoints You do not pass any parameters to the endpoints. Most of the endpoints would yield a 500 Internal Server Error because of that, if they worked at all. The following snippet from your code illustrates that problem

"Operation Accepted and Container Deleted" in {
  Post("delete") -> routes -> check {
    status === StatusCodes.ACCEPTED
  }
}
"Internal Server Error while deleting container" in {
  Post("delete") -> routes -> check {
    status === StatusCodes.INTERNAL_SERVER_ERROR
  }
}

Here you are calling the exact same endpoint (/delete) twice, but still expect different results, namely the StatusCode ACCEPTED and INTERNAL_SERVER_ERROR. You can have a look into the swagger documentation to find out what endpoints needs query parameters, and what endpoints need body parameters (e.g. /register).

There are some codestyle errors Namely the package name was declared as de.upb.cs.swt.delphi.webapi, while the correct package name would be de.upb.cs.swt.delphi.registry.connection. Also the file is named ServerTest.scala while the class inside the file is named ServerTests.

Currently these tests do not add any coverage at all. They all pass, but only because they are never being executed. Please read through my feedback here carefully, maybe have a look into some more tutorials on using the akka-http-testkit, and change your implementation accordingly.

[Ayybeeshafi]

@johannesduesing Thank you for such a detailed response. It is making a lot more sense now. My confusion was how are these tests passing if I am not right. That is too cleared. I will start working to change the implementation.

[Ayybeeshafi]

@johannesduesing I am unable to figure out correct parameters for LinksFrom() and LinksTo(). If you can look into it in your free time. Thank you.

[johannesduesing]

I've fixed the issues with those two cases and also improved the coverage by adding some tests for the default error handling of docker operations. Will wait for the CI build and than merge your PR :+1:

[johannesduesing]

Closed with #52