add new API GetEphemerals #45
Conversation
Codecov Report
@@ Coverage Diff @@
## master #45 +/- ##
==========================================
+ Coverage 76.11% 76.21% +0.09%
==========================================
Files 7 7
Lines 1189 1198 +9
==========================================
+ Hits 905 913 +8
Misses 195 195
- Partials 89 90 +1
Continue to review full report at Codecov.
|
|
How to fix the 'Codecov Report'? |
|
It's probably fine, but we need a better way to handle unit testing in this package in general. There are integration tests which may be good for this as well? |
|
What should I do next? Waiting? close? Because I want to add more new API of ZK 3.6, your project is well written, clear and easy to read |
conn.go
Outdated
| } | ||
|
|
||
|
|
||
| // Synchronously gets all the ephemeral nodes matching `path` created by this session. |
There was a problem hiding this comment.
This comment needs to match the rest and should also match Go best practices (i.e using complete sentences, starting with the method name). Argument and returns here should be obvious and don't need documentation.
All calls are synchronous in this library so that is also not needed.
https://golang.org/doc/effective_go.html#commentary
Also can you place this method near the rest of the request methods? Maybe around Get?
There was a problem hiding this comment.
Sure~I will revise it according to your suggestion
| case <-okChan: | ||
| case <-time.After(3 * time.Second): | ||
| t.Fatal("apparent deadlock!") | ||
| } |
There was a problem hiding this comment.
At that time, I wrote a unit test for GetEphemerals, which uses the ZK service of direct connection 127.0.0.1. This requires me to start a ZK service locally. However, after the first two commits were submitted, GitHub's default unit test check failed directly. So I tried to remove this unit test and verify whether the automatic check would pass. Obviously, another problem I mentioned above appeared One question (How to fix the 'Codecov Report'?) Maybe I should comment on the test? Because it seems that there is no corresponding test method for other public methods
|
My latest modification has been submitted. Obviously, ZK in version 3.5 can't run unittest, because the API comes from version 3.6. |
|
We can't land broken tests. We should create a new test file gaurded by build flags. If you aren't comfortable with that I can code it up real quick and then rebase this on top? |
|
Of course, waiting for your good news |
|
How's going on |
|
Sorry I am on a "mini vacation" between jobs right now. No ETA yet. |
|
Take it easy. I'm not pushing you |
2 participants
Try to add new API for zk 3.6, this commit is just for
GetEphemerals