-
Notifications
You must be signed in to change notification settings - Fork 34
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
Adds additional server definitions tests #23
Conversation
b3ebcda
to
55bb9d2
Compare
55bb9d2
to
77198b3
Compare
Codecov Report
@@ Coverage Diff @@
## master #23 +/- ##
===========================================
+ Coverage 34.78% 48.93% +14.15%
===========================================
Files 7 8 +1
Lines 46 47 +1
===========================================
+ Hits 16 23 +7
+ Misses 30 24 -6
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, just left some comments. Maybe we could add an interpreter ServerConfig.Op ~> Id
for the tests.
"work as expected" in { | ||
|
||
val configList: List[GrpcConfig] = List(AddService(sd1)) | ||
val server: Server = SServerBuilder(port).withGrpcConfigList(configList).buildServer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just:
SServerBuilder(port, configList)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this test is covering withGrpcConfigList
branch ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
val serverF = BuildServerFromConfig[ServerConfig.Op]("rpc.server.port").interpret[Future] | ||
|
||
serverF map (_.getPort shouldBe port) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this is a Future
, this test is not verifying the value. I've tried different values for port
and it succeeds for all of them
|
||
"load the default port when the config port path is not found" in { | ||
|
||
val serverF = BuildServerFromConfig[ServerConfig.Op]("rpc.wrong.path").interpret[Future] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
193d7bf
to
43ab1ec
Compare
43ab1ec
to
2e528e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR completes #15
Now, rpc server module reaches almost the 100% of test coverage. In the next PRs I'll cover the client definition tests.
Please, @fedefernandez could you take a look? Thanks!