269 update core proto #346
Conversation
| _core_service->stop(); | ||
| uuids_stream_future.wait(); | ||
|
|
||
| EXPECT_EQ(3, uuids.size()); |
There was a problem hiding this comment.
Nitpick, this would have to be an ASSERT_EQ because the at() below will fail otherwise.
There was a problem hiding this comment.
Good point :)
|
|
||
| std::future<void> CoreServiceImplTest::subscribeTimeoutAsync(int *stream_count) | ||
| { | ||
| return std::async(std::launch::async, [this, stream_count]() { |
There was a problem hiding this comment.
Hm, I didn't know `std::launch::async.
JonasVautherin
force-pushed
the
269-update-core-proto
branch
from
7e9d701 to
33d8aa8
Compare
|
Well, 13 unit tests for 3 rpc functions, that doesn't sound excessive to me :-). For gRPC streams, it requires quite some code because I run an in-process server. Also, only the public interface is unit tested, so that should be quite stable. If the tests ever need big changes, it will mean that the public interface changed, and in this case I find it reasonable to have to change the tests. I know it may seem like a lot of tests, but I am quite confident now that the code is doing what I expect it to do (even though it doesn't mean it is completely bug-free). If somebody ever refactors the code without breaking the tests, I will be confident that the refactoring is correct. And finally, I believe the tests serve as documentation: one can learn how to interact with the code by reading the unit tests. |
JonasVautherin
force-pushed
the
269-update-core-proto
branch
from
33d8aa8 to
21a449f
Compare
JonasVautherin
force-pushed
the
269-update-core-proto
branch
from
21a449f to
39535df
Compare
This PR implements
SubscribeDiscoverandSubscribeTimeoutfor the core module.Fixes #269.