test(08-wasm): added 'TestInitialize' using mockVM #4894
Conversation
This reverts commit 738f8b8.
srdtrk
requested review from
colin-axner,
AdityaSripal and
damiannolan
as code owners
| const ( | ||
| tmClientID = "07-tendermint-0" | ||
| grandpaClientID = "08-wasm-0" | ||
| tmClientID = "07-tendermint-0" | ||
| wasmClientID = "08-wasm-0" | ||
| ) |
There was a problem hiding this comment.
At one point, I was using grandpaClientID in my test (I'm not anymore). So I had renamed it to wasmClientID. I can revert this if requested.
There was a problem hiding this comment.
Curious, how come you reverted? I think wasmClientID makes more sense, doesn't it?
There was a problem hiding this comment.
It made this PR harder to review due to large diff. I want to make a new PR for this
| // tmConsensusStateData, err := suite.chainA.Codec.MarshalInterface(tmConsensusState) | ||
| // suite.Require().NoError(err) | ||
| func (suite *TypesTestSuite) TestInitialize() { | ||
| panicMsg := errors.New("panic in InstantiateFn") |
There was a problem hiding this comment.
I'm unsure if we should bother testing for a panic... what do you think? We should never have to handle a panic from the vm inside of our code, right? Aren't all panics caught here within the actual vm and propagated as errors?
Would probably simplify the test structure if we didn't have to include this, but either way test looks good!
There was a problem hiding this comment.
Since wasm engine is an interface now, I think it should be in our scope to include basic tests that might break certain assumptions about wasmvm. Also, I'm not sure if they panic later again somewhere in their code
There was a problem hiding this comment.
I'd agree with Damian here. From what I can understand, the idea is to test that panics are propagated across the boundary (which might not be FFI like wasmvm)? I'm trying to understand the benefit of testing for this, especially given that wasmvm appears to translate em all into err's i.e if we plugged wasmvm here, it would fail.
W/e the case, happy to keep on this path if there's consensus!
There was a problem hiding this comment.
cc. @chatton @charleenfei @colin-axner any thoughts/preference? I didn't implement panic tests in #4923 yet but if there's consensus to do it I will push an update.
I'd see more of an argument to testing for panics if we were actually recovering from them in client state methods, but we aren't, if a panic occurs across the api boundary here its just going to propagated to baseapp anyways.
I'd be happy to skip the panic tests as it simplifies the structure and is easier to read and maintain in the long run
There was a problem hiding this comment.
Yeah, I guess the only benefit I see to implementing panic tests is making sure that there are no panic recoveries anywhere in the stack between callbackFn and 02-client cc. @damiannolan
There was a problem hiding this comment.
sorry late to this thread, but i told @damiannolan my thoughts in person (and in my original discussion on this topic with @srdtrk), agree with skipping panic tests.
| const ( | ||
| tmClientID = "07-tendermint-0" | ||
| grandpaClientID = "08-wasm-0" | ||
| tmClientID = "07-tendermint-0" | ||
| wasmClientID = "08-wasm-0" | ||
| ) |
There was a problem hiding this comment.
Curious, how come you reverted? I think wasmClientID makes more sense, doesn't it?
| @@ -82,6 +82,9 @@ func (suite *TypesTestSuite) SetupWasmWithMockVM() { | |||
| suite.coordinator = ibctesting.NewCoordinator(suite.T(), 1) | |||
| suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1)) | |||
|
|
|||
| suite.ctx = suite.chainA.GetContext().WithBlockGasMeter(storetypes.NewInfiniteGasMeter()) | |||
| suite.store = suite.chainA.App.GetIBCKeeper().ClientKeeper.ClientStore(suite.ctx, grandpaClientID) | |||
There was a problem hiding this comment.
I'd be in favour of switching var name or having another const DefaultWasmClientID or similar
There was a problem hiding this comment.
Makes sense, will be making another PR for this
Description
This PR:
TestInitializeusing mockVMgrandpaClientIDtowasmClientIDcloses: #4825
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.