-
Notifications
You must be signed in to change notification settings - Fork 4.1k
REVIEW: baseapp: start TestInfo #488
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
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #488 +/- ##
===========================================
+ Coverage 53.89% 56.03% +2.14%
===========================================
Files 27 27
Lines 1542 1542
===========================================
+ Hits 831 864 +33
+ Misses 660 625 -35
- Partials 51 53 +2 |
adrianbrink
left a comment
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.
LGTM
|
surely, someone can merge this. |
ethanfrey
left a comment
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.
Good tests in general.
I wonder about glide.lock (see comment).
Also, while we test messages, we have no meaningful unit tests for handler. That would be very good to add (but can be a separate PR). For an example, see:
https://github.com/tendermint/clearchain/blob/master/types/handler_test.go#L105-L171
| @@ -0,0 +1,151 @@ | |||
| hash: 74ba16fcb7dac2ceca406dcc6c6ed00ab1cd09af4ca6f3f1d35452254e149fb4 | |||
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 do we have this glide.lock here?
Isn't it better just to use the glide.lock in the root directory.
It makes it very hard to upgrade and test code with basecoin otherwise, especially as the repo is in a large state of flux now.
|
@jolesbi Only Jae and Bucky can merge on this repo. |
No description provided.