Panic when testRuntimeInterface methods need hooks but lack them #1399
Conversation
| @@ -409,22 +409,37 @@ func (i *testRuntimeInterface) Hash(data []byte, tag string, hashAlgorithm HashA | |||
| } | |||
|
|
|||
| func (i *testRuntimeInterface) SetCadenceValue(owner common.Address, key string, value cadence.Value) (err error) { | |||
| if i.setCadenceValue == nil { | |||
| panic("must specify testRuntimeInterface.setCadenceValue") | |||
There was a problem hiding this comment.
Good idea! Maybe even do this consistently for all functions, currently some just return a default of no hook is provided. Better to have to explicitly provide functions that are called in the tests
There was a problem hiding this comment.
Ah I just switched some to having defaults! I can switch them all over though. I agree that being explicit is better since defaults tend to hide bugs.
There was a problem hiding this comment.
Agreed, better to switch to panics, it's a great idea!
There was a problem hiding this comment.
A LOT of tests fail when I eliminate the defaults. I created #1400 so we can account for the work and maybe assign it later depending on priority. It's a constrained enough problem that I should be able to it without needing much if any help but I don't want to push more urgent stuff back.
However, there are a few more methods that currently panic with a generic message instead of a nice one. I'm going to add them to this PR before asking for another review (hopefully super quick) then merging into master.
There was a problem hiding this comment.
Sounds good! Thank you for trying and opening the separate PR 👍
|
@robert-e-davidson3 Please follow the checklist in the PR template as well as you can. It helps with avoiding mistakes, labeling helps with generating the release notes, etc. Thank you! |
robert-e-davidson3
force-pushed
the
robert/panic-when-runtime-tests-are-missing-needed-hooks
branch
from
a128c91 to
1ac03c9
Compare
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:master commit 8ecdaf3 Results
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
robert-e-davidson3
force-pushed
the
robert/panic-when-runtime-tests-are-missing-needed-hooks
branch
from
117d5c1 to
1ac03c9
Compare
| @@ -409,22 +409,37 @@ func (i *testRuntimeInterface) Hash(data []byte, tag string, hashAlgorithm HashA | |||
| } | |||
|
|
|||
| func (i *testRuntimeInterface) SetCadenceValue(owner common.Address, key string, value cadence.Value) (err error) { | |||
| if i.setCadenceValue == nil { | |||
| panic("must specify testRuntimeInterface.setCadenceValue") | |||
There was a problem hiding this comment.
Sounds good! Thank you for trying and opening the separate PR 👍
Codecov Report
@@ Coverage Diff @@
## master #1399 +/- ##
=======================================
Coverage 73.08% 73.08%
=======================================
Files 280 280
Lines 38943 38943
=======================================
Hits 28463 28463
Misses 9037 9037
Partials 1443 1443
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
SupunS
deleted the
robert/panic-when-runtime-tests-are-missing-needed-hooks
branch
Description
The runtime test interface has hooks for the FVM methods of the "Interface" API. Most have defaults for when hooks aren't set. A few instead throw
invalid memory address or nil pointer dereference. This PR gives them defaults where appropriate else a more descriptive panic.Update: Switching to make all of the methods panic when they don't have a hook.
masterbranchFiles changedin the Github PR explorer