types: ConsensusParams test + document the ranges/limits by odeke-em · Pull Request #748 · tendermint/tendermint

odeke-em · 2017-10-15T21:35:43Z

Fixes #747
Updates #693

Document the unmentioned limits for ConsensusParams.Validate()
Make the limit for ConsensusParams.BlockSizeParams.MaxBytes
clear at 100MiB

codecov-io · 2017-10-15T21:56:21Z

Codecov Report

❗ No coverage uploaded for pull request base (develop@fa56e8c). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop     #748   +/-   ##
==========================================
  Coverage           ?   57.24%           
==========================================
  Files              ?       82           
  Lines              ?     8436           
  Branches           ?        0           
==========================================
  Hits               ?     4829           
  Misses             ?     3191           
  Partials           ?      416

melekes · 2017-10-16T07:23:13Z

types/params.go


 const (
-	maxBlockSizeBytes = 104857600 // 100MB
+	_100MiB = 104857600


👎 we're losing meaning here

The problem though is that a plain maxBlockSizeBytes itself requires having to look it up and isn't documented anywhere, IMHO _100MiB is self documenting.

I agree, but how about the following: what if somebody else starts using _100MiB and later you decide to change it to _120MiB. you accidentally change both usages and break the code (or worse).

I think having to look it up is fine. We shouldn't ruin the variable name :P

We do need to document this somewhere though.

Aye aye, I'll revert the variable.

melekes · 2017-10-16T07:24:55Z

types/params.go


 // Validate validates the ConsensusParams to ensure all values
 // are within their allowed limits, and returns an error if they are not.
+// Expecting:


why duplicate the code? is this what we want from documentation? step by step duplication of the code

we should probably document maxBlockSizeBytes somewhere here https://tendermint.readthedocs.io/en/master/specification/block-structure.html

I don't think this is duplicating the code, it saves folks from having to open the code and having to read each line, by line. That's what am trying to avoid. I had to go through the guts of it in order to write the tests themselves.

I had to go through the guts of it in order to write the tests themselves.

well, you are a developer, aren't you? you have to go through the guts sometimes

Perhaps a better alternative is to include these conditions in the comments on the Params struct itself?

Could you provide a similar example from Golang source code?

well, you are a developer, aren't you? you have to go through the guts sometimes

LOL I don't think this is fair in this case - these conditions are undocumented, so from the perspective of being able to test the functionality exposed to the user, it's fair to say this was to0 opaque - but for that reason we should put it on the Params struct rather than here on the Validate function

well, you are a developer, aren't you? you have to go through the guts sometimes
I believe that the purpose of us making APIs is so make it easy for other people to build off our stuff easily. Imagine you had to read through all the source code of Go's APIs just to be able to use a function? My motivation here it is to try to improve developer experience/productivity and reduce the cognitive overload.

Could you provide a similar example from Golang source code?
I can find tonnes of examples:

https://github.com/golang/go/blob/a6ae01a64a6a5ef17a4825cd486aa31f62481100/src/net/http/request.go#L1146-L1147

https://github.com/golang/go/blob/a9afa4e933f3eff131f12e24bb0f5b9f3168ca14/src/os/dir.go#L7-L46
https://github.com/golang/go/blob/a9afa4e933f3eff131f12e24bb0f5b9f3168ca14/src/os/env.go#L46

https://github.com/golang/go/blob/a9afa4e933f3eff131f12e24bb0f5b9f3168ca14/src/os/exec/exec.go#L127-L139

https://github.com/golang/go/blob/a6ae01a64a6a5ef17a4825cd486aa31f62481100/src/runtime/mksizeclasses.go#L171-L174

https://github.com/golang/go/blob/a6ae01a64a6a5ef17a4825cd486aa31f62481100/src/io/io.go#L286-L294

https://github.com/golang/go/blob/a6ae01a64a6a5ef17a4825cd486aa31f62481100/src/io/io.go#L296-L302

https://github.com/golang/go/blob/a6ae01a64a6a5ef17a4825cd486aa31f62481100/src/io/io.go#L330-L336

https://github.com/golang/go/blob/a6ae01a64a6a5ef17a4825cd486aa31f62481100/src/bytes/bytes.go#L115-L119

https://github.com/golang/go/blob/a6ae01a64a6a5ef17a4825cd486aa31f62481100/src/math/big/rat.go#L463-L468

and many more.

@ebuchman Perhaps a better alternative is to include these conditions in the comments on the Params struct itself?
Perhaps, but I think it is very easy for code to go stale that way as it is detached from the actual validation code.

oh, wow. sorry for being so harsh. admit that I was wrong. great examples! Agree that good API and dev experience/productivity are very important.

Not a biggie @melekes :)

melekes · 2017-10-16T07:27:52Z

types/params_test.go

+)
+
+func TestConsensusParamsValidation(t *testing.T) {
+	c1 := new(types.ConsensusParams)


why not table tests?

This seemed simpler, usually I'd use table tests but here they seemed simpler. However, I can add table tests since they aren't tightly coupled to the previous lines and don't require saving context of what previously happened. Thanks for pointing this out :)

Fixes #747 Updates #693 * Document the unmentioned limits for ConsensusParams.Validate() * Make the limit for ConsensusParams.BlockSizeParams.MaxBytes clear at 100MiB

odeke-em · 2017-10-16T22:58:57Z

@melekes I've made them table tests, PTAL.

ebuchman

By the way, did you see this: https://github.com/tendermint/tendermint/blob/master/types/genesis_test.go#L72 ?

Is there much new here?

ebuchman · 2017-10-17T07:19:21Z

types/params.go


 // Validate validates the ConsensusParams to ensure all values
 // are within their allowed limits, and returns an error if they are not.
+// Expecting:


Perhaps a better alternative is to include these conditions in the comments on the Params struct itself?

ebuchman · 2017-10-17T07:19:49Z

types/params.go


 	// ensure blocks aren't too big
-	if params.BlockSizeParams.MaxBytes > maxBlockSizeBytes {
+	if params.BlockSizeParams.MaxBytes > _100MiB {


this isn't good because if we change the limit we have to change the variable name. it really should just be maxBlockSizeBytes

Great point here, thanks.

odeke-em · 2017-10-18T15:37:51Z

By the way, did you see this: https://github.com/tendermint/tendermint/blob/master/types/genesis_test.go#L72 ?
No I hadn't, perhaps we could update the test name, I grepped for Validate and no hits in the tests, otherwise I have to go through each file checking

Is there much new here?
Just the documentation, plus the actual new test cases testing the extreme ends of the ranges as opposed to 0, 1, but these can be adapted into that test. Thank you for pointing out that duplicate.

ebuchman · 2017-10-19T00:49:04Z

perhaps we could update the test name

Yes, or just move that test to params_test.go, which is prob more appropriate. Thanks

odeke-em · 2017-10-20T06:10:09Z

Alright, thanks for the initial review folks. I've updated the PR with your feedback, PTAL at f24f039.

* Updated changelog with API changes in tendermint#286 * Cosmetic * Update .changelog/unreleased/breaking-changes/286-p2p.md Co-authored-by: Daniel <daniel.cason@informal.systems> * unclog build > CHANGELOG.md --------- Co-authored-by: Daniel <daniel.cason@informal.systems>

…#1120) (tendermint#1122) * node: Revert removal of public reactor accessors (tendermint#1120) * Revert "Remove unused code (tendermint#286)" This reverts commit a2d9915. Signed-off-by: Thane Thomson <connect@thanethomson.com> * node: Remove access to consensus state Consensus state should only ever be accessible via the consensus reactor. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add changelog entry Signed-off-by: Thane Thomson <connect@thanethomson.com> * Fix mistake in changelog entry Signed-off-by: Thane Thomson <connect@thanethomson.com> --------- Signed-off-by: Thane Thomson <connect@thanethomson.com> (cherry picked from commit 577cbcb) * Revert "Updated changelog with API changes in tendermint#286 (tendermint#748)" This reverts commit 7a253f1. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Rebuild changelog with unreleased entries Signed-off-by: Thane Thomson <connect@thanethomson.com> --------- Signed-off-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: Thane Thomson <connect@thanethomson.com>

odeke-em requested a review from ebuchman as a code owner October 15, 2017 21:35

odeke-em requested a review from zramsay October 15, 2017 21:35

zramsay approved these changes Oct 16, 2017

View reviewed changes

melekes reviewed Oct 16, 2017

View reviewed changes

types: ConsensusParams test + document the ranges/limits

bff069f

Fixes #747 Updates #693 * Document the unmentioned limits for ConsensusParams.Validate() * Make the limit for ConsensusParams.BlockSizeParams.MaxBytes clear at 100MiB

odeke-em force-pushed the params-test branch from 14b7821 to bff069f Compare October 16, 2017 22:58

ebuchman suggested changes Oct 17, 2017

View reviewed changes

types: ConsensusParams: add feedback from @ebuchman and @melekes

f24f039

odeke-em force-pushed the params-test branch from c6e5fb7 to f24f039 Compare October 20, 2017 06:11

ebuchman approved these changes Oct 23, 2017

View reviewed changes

ebuchman merged commit f97229f into develop Oct 23, 2017

ebuchman deleted the params-test branch October 23, 2017 15:18

Conversation

odeke-em commented Oct 15, 2017

Uh oh!

codecov-io commented Oct 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odeke-em commented Oct 16, 2017

Uh oh!

ebuchman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odeke-em commented Oct 18, 2017

Uh oh!

ebuchman commented Oct 19, 2017

Uh oh!

odeke-em commented Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Oct 15, 2017 •

edited

Loading

odeke-em commented Oct 20, 2017 •

edited

Loading