Conversation
- Fixed bug in actors where downloads were being retried when they shouldn't - Added type assertions to internaltypes - Fixed empty func in peer.go
consensus/db/db.go
Outdated
| func (db *Database) SetSafeToProceed(txn *badger.Txn, height uint32, isSafe bool) error { | ||
| if height%constants.EpochLength != 1 { | ||
| panic("The height must be mod 1 epoch length") | ||
| panic(fmt.Errorf("the height must be mod 1 epoch length. height: %v\n", height)) |
There was a problem hiding this comment.
nit: panic internally just converts the error to a string, which negates the need to do fmt.Errorf. You can just do a fmt.Sprintf if you want to convert to format a string. Also the trailing \n isn't needed as panic inserts one automatically.
There was a problem hiding this comment.
Good point, I'll improve this on the next commit
consensus/db/db.go
Outdated
| func (db *Database) SetSafeToProceed(txn *badger.Txn, height uint32, isSafe bool) error { | ||
| if height%constants.EpochLength != 1 { | ||
| panic("The height must be mod 1 epoch length") | ||
| panic(fmt.Errorf("the height must be mod 1 epoch length. height: %v\n", height)) |
There was a problem hiding this comment.
nit: panic internally just converts the error to a string, which negates the need to do fmt.Errorf. You can just do a fmt.Sprintf if you want to convert to format a string. Also the trailing \n isn't needed as panic inserts one automatically.
| skipCallCheck bool | ||
| } | ||
|
|
||
| // assert struct `testingProxy` implements `reqBusView` , `txMarshaller`, `databaseView` and `typeProxyIface` interfaces |
There was a problem hiding this comment.
question: Does it matter if testingProxy doesn't implement these interfaces if tests cover that functionality?
There was a problem hiding this comment.
Some parts of the code require interfaces to be passed in as arguments and this ensures the testingProxy struct actually implements the required interfaces. Note that testingProxy mocks functionality to better test these components in isolation.
| var _ databaseView = &testingProxy{} | ||
| var _ typeProxyIface = &testingProxy{} | ||
|
|
||
| func (trb *testingProxy) checkExpect() error { |
There was a problem hiding this comment.
nit: Might be a good idea to convert these functions into test helpers (take a *t.Testing and call t.Helper() at the start) so that you can just t.Error and t.Fatal without having to worry about returning errors. Less handling on the calling test side and the test log will bubble up the checks to the failing parent test.
There was a problem hiding this comment.
That's an interesting idea and I see its potential in more functions like generateChain() or makeGoodBlock(). This particular test suite would require a great deal of rewriting to accommodate that, given its stateful nature. I'll investigate more about helper functions and learn how we can leverage them in our testing.
- Improved panic error messages
|
SonarCloud Quality Gate failed.
|
2 participants
Scope
What is changing with this PR?
Added lots of test the the admin handler and dman packages.
Why?
To ensure greater stability on AliceNet