Fix event handling - Closes #4208 #4211
Conversation
framework/src/modules/chain/chain.js
Outdated
| return; | ||
| } | ||
| } catch (error) { | ||
| this.logger.info( |
There was a problem hiding this comment.
Shouldn't the log level of this error be debug or error?
There was a problem hiding this comment.
maybe we can remove data to be logged as it clutters up the logs if someone constantly sends malformed data
There was a problem hiding this comment.
I thought error will be too much since it's handled properly, but user should know about those event.
Maybe info is too low on this case, so i increased it to warn
| if (event === 'postBlock') { | ||
| await this.transport.postBlock(data); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Nit: return at the end instead of returning each condition?
There was a problem hiding this comment.
It should be fine, but I think in this case it's bit more clear it doesn't go to other cases
There was a problem hiding this comment.
I agree with @ManuGowda. Using else if + 1 single return is cleaner.
| if (event === 'postBlock') { | ||
| await this.transport.postBlock(data); | ||
| return; | ||
| } |
There was a problem hiding this comment.
I agree with @ManuGowda. Using else if + 1 single return is cleaner.
| @@ -32,7 +32,7 @@ describe('WS transport', () => { | |||
| let p2p; | |||
|
|
|||
| function postTransaction(transactionToPost) { | |||
| p2p.send({ | |||
| return p2p.send({ | |||
There was a problem hiding this comment.
Do we really need this return? I actually don't like the structure of the entire describe. I suggest to clean it:
- Its
beforeEachis misleading - Remove the use of done.
- The inner
describeblock is not necessary - I would remove
postTransaction()to ease the understanding of every test or move it to a more general handler. If you don't agree, we should follow this pattern later on and do the same thing when sending.
There was a problem hiding this comment.
This PR is not intended to refactor the test.
All of the functional tests will be recovered in jest, so not needed to rewrite for this patch imo
What was the problem?
How did I solve it?
How to manually test it?
Review checklist