refactor: replaces events type const - #621
Conversation
Codecov Report
@@ Coverage Diff @@
## master #621 +/- ##
==========================================
+ Coverage 21.40% 22.09% +0.69%
==========================================
Files 127 127
Lines 6353 6353
==========================================
+ Hits 1360 1404 +44
+ Misses 4917 4872 -45
- Partials 76 77 +1
Continue to review full report at Codecov.
|
08fba9c to
1e677a5
Compare
boz
left a comment
boz
left a comment
There was a problem hiding this comment.
Thanks so much for adding testing. I added some comments below - it's not clear to me why the tests are passing :-/
In addition to what you have, it'd be nice to test that encode/decode are inverse of one another if possible:
parse(event.ToSDKEvent()) == event
| Type: sdkutil.EventTypeMessage, | ||
| Module: ModuleName, | ||
| }, | ||
| expErr: nil, |
There was a problem hiding this comment.
Looking at this, I believe these things should cause an error:
Actionis unpopulatedActionis neitherevActionDeploymentCreatenorevActionDeploymentUpdateDeploymentIDattributes are not set.
There was a problem hiding this comment.
Yeah... I don't know how I missed that, and clearly the err assertion isn't working.
There was a problem hiding this comment.
I think LAMBDA BOWL is causing one of those annoying closure-in-a-loop bugs.
1af6a80 to
ddfe149
Compare
| ) | ||
|
|
||
| var ( | ||
| keyAcc, _ = sdk.AccAddressFromBech32("akash1qtqpdszzakz7ugkey7ka2cmss95z26ygar2mgr") |
There was a problem hiding this comment.
good idea. maybe can have a thing in testutil w/ like 100 hard-coded addresses.
There was a problem hiding this comment.
Yeah I just pulled that out of local setup.
|
|
||
| func (tep testEventParsing) testMessageType() func(t *testing.T) { | ||
| _, err := ParseEvent(tep.msg) | ||
| return func(t *testing.T) { |
There was a problem hiding this comment.
I'm guessing that you're returning a closure so that t.Run() executes in parallel... but t.Parallel() is not called and ParseEvent() wouldn't be run in it anyways.
I think just putting all of this in the TestEventParsing function instead of as a method would make this a lot easier to understand.
Or if you really want to use a method, put the closure in TestEventParsing instead of here.
There was a problem hiding this comment.
This pattern is purely due to the linter being tyranical about what state can be referenced in the main t.Run(...) function. Nothing to do with t.Parallel().
Linter is harsh...
* Previously used the sdk.EventTypeMessage == "message" string to
identify all of our messages. This provided little value.
* Updated all ephemeral messages to be prefixed with
"sdkutil.EventTypeMessage" == "akash.v1".
* Added tests to assert successful event parsing.
fixes #607
Signed-off-by: Josh Roppo <josh@akash.network>
ddfe149 to
ee8ba2d
Compare
4 participants
identify all of our messages. This provided little value.
"sdkutil.EventTypeMessage" == "akash.v1".
there are no parsing errors right now.
fixes #607