platisd / indoor-navigation-system

[WIP] An indoor navigation system to guide users towards their colleagues' desks
Apache License 2.0
15 stars 13 forks source link

Low ins-service (server) coverage #78

Open platisd opened 6 years ago

platisd commented 6 years ago

Description

INS service has low code coverage (i.e. ins_service.cpp) which can result into defects and low internal quality.

Definition of Done

The code coverage is raised to at least 70%. This could require some code refactoring.

platisd commented 6 years ago

Current coverage can be seen in codecov.

platisd commented 6 years ago

I had a look today and I believe we need to use a mock of Pistache instead of Pistache itself which is being linked in our test cmake, in order to check those callbacks. I don't think (many) production code changes will be necessary. For example, once we mock Pistache functions, i.e. routes::post and get (I don't remember if these are the exact names, just making a point) we can use gtest's SaveArg to save the private callback in a local std::function variable and then call it directly from our test.

platisd commented 6 years ago

My idea is to deploy the following tactic:

Create the necessary header files (e.g. pistache/endpoint.h) which will practically be stubs that are just calling a PistacheMock singleton. Then in our tests we can state our expectations on our singleton mock which will be indirectly used by the production code.

It is the same method used for creating mocks for the ins-node.

samup4web commented 6 years ago

I will have to take a look at how it works in ins-node to fully grab the concept, but it sounds like a good approach. I have not had much time to settle down with the project lately :disappointed: Have to do something about that.

platisd commented 6 years ago

No worries! These are mental notes mainly for myself. :+1: 😃

platisd commented 6 years ago

I was looking at this yesterday as well and I think I am going the wrong way. I noticed the wrapper_lib is where the Pistache library should be abstracted into if we are to follow the same pattern that we've been using so far for testing. What do you think?

samup4web commented 6 years ago

Well, that is the pattern I thought would work best. It is a simple one as well, the only overhead is that it requires a wrapper class and function for each calls we intend to mock/test, and also the production code has to be modified for the testing purpose as it has to use the wrapper instead of directly using the library. If I remember correctly, this almost worked until I had an strange issue with some object instance in the library. I think it has to do with Pistache::Rest::Request or the Pistache::Rest::Routes. Actually, it will be interesting to revisit this problem again.

lexious89 commented 5 years ago

is this still an issue?

platisd commented 5 years ago

Yeap :(