Add a custom vm.Run() function #9

Merged
fyrchik merged 5 commits from elebedeva/contract-coverage-primer:covertest/run into master 2024-09-04 19:51:22 +00:00
2 changed files with 133 additions and 1 deletions

58
covertest/run.go Normal file
View file

@ -0,0 +1,58 @@
package covertest
import (
"errors"
"github.com/nspcc-dev/neo-go/pkg/util"
"github.com/nspcc-dev/neo-go/pkg/vm"
"github.com/nspcc-dev/neo-go/pkg/vm/opcode"
"github.com/nspcc-dev/neo-go/pkg/vm/vmstate"
)
// InstrHash maps instruction with scripthash of a contract it belongs to.
fyrchik marked this conversation as resolved Outdated

Instruction doesn't have a script hash, a contract it belongs to does.

Instruction doesn't have a script hash, a _contract it belongs to_ does.

Fixed

Fixed
type InstrHash struct {
Offset int
fyrchik marked this conversation as resolved Outdated

Position or Offset would be more descriptive IMO (each opcode has a "number" which is Instruction here)

`Position` or `Offset` would be more descriptive IMO (each opcode has a "number" which is `Instruction` here)

Fixed

Fixed
Instruction opcode.Opcode
ScriptHash util.Uint160
}
// Run starts execution of the loaded program and accumulates all seen opcodes
// together with the scripthash of a contract they belong to.
// Original vm.Run(): https://github.com/nspcc-dev/neo-go/blob/v0.101.3/pkg/vm/vm.go#L418
fyrchik marked this conversation as resolved Outdated

Could you give a permalink to the original code of this function in comment?
It would benefit both reviewers and anyone who touches this code in a year.

Could you give a permalink to the original code of this function in comment? It would benefit both reviewers and anyone who touches this code in a year.

Fixed

Fixed
func Run(v *vm.VM) ([]InstrHash, error) {
fyrchik marked this conversation as resolved
Review

First line of the function should not be empty (there is a linter for this, we just don't have it in this repo)

First line of the function should not be empty (there is a linter for this, we just don't have it in this repo)
Review

Fixed

Fixed
if !v.Ready() {
return nil, errors.New("no program loaded")
}
if v.HasFailed() {
// VM already ran something and failed, in general its state is
// undefined in this case so we can't run anything.
return nil, errors.New("VM has failed")
}
// vmstate.Halt (the default) or vmstate.Break are safe to continue.
var ops []InstrHash
for {
switch {
case v.HasFailed():
// Should be caught and reported already by the v.Step(),
// but we're checking here anyway just in case.
return ops, errors.New("VM has failed")
case v.HasHalted(), v.AtBreakpoint():
// Normal exit from this loop.
return ops, nil
case v.State() == vmstate.None:
nStr, curInstr := v.Context().NextInstr()
ops = append(ops, InstrHash{
Offset: nStr,
Instruction: curInstr,
ScriptHash: v.Context().ScriptHash(),
})
if err := v.Step(); err != nil {
return ops, err
}
default:
return ops, errors.New("unknown state")
}
}
}

View file

@ -1,18 +1,26 @@
package contract
import (
"errors"
"math/rand"
"path"
"testing"
"git.frostfs.info/TrueCloudLab/contract-coverage-primer/covertest"
"github.com/davecgh/go-spew/spew"
"github.com/nspcc-dev/neo-go/pkg/compiler"
"github.com/nspcc-dev/neo-go/pkg/neotest"
"github.com/nspcc-dev/neo-go/pkg/neotest/chain"
"github.com/nspcc-dev/neo-go/pkg/smartcontract/callflag"
"github.com/nspcc-dev/neo-go/pkg/smartcontract/trigger"
"github.com/nspcc-dev/neo-go/pkg/vm"
"github.com/nspcc-dev/neo-go/pkg/vm/stackitem"
"github.com/stretchr/testify/require"
)
const ctrPath = "../impulse"
// Key for tests
// keys for tests
var (
validKey = []byte{1, 2, 3, 4, 5}
invalidKey = []byte{1, 2, 3}
@ -41,3 +49,69 @@ func TestContract(t *testing.T) {
inv.InvokeFail(t, "Invalid key size", "putNumber", invalidKey, 42)
inv.InvokeFail(t, "Invalid key size", "getNumber", invalidKey)
}
func TestRun(t *testing.T) {
e := newExecutor(t)
ctrDI := covertest.CompileFile(t, e.CommitteeHash, ctrPath, path.Join(ctrPath, "config.yml"))
e.DeployContract(t, ctrDI.Contract, nil)
fyrchik marked this conversation as resolved
Review

101 and 2 look like magic constants. Can we calculate them somehow?

`101` and `2` look like magic constants. Can we calculate them somehow?
Review

Fixed

Fixed
startOffsetPutNumber, err := getStartOffset(ctrDI.DebugInfo, "PutNumber")
fyrchik marked this conversation as resolved Outdated

We use spew for debug-printing big structs. In other cases (tests) t.Log or t.Logf might be more appropriate.

We use `spew` for debug-printing big structs. In other cases (tests) `t.Log` or `t.Logf` might be more appropriate.

Fixed

Fixed
require.NoError(t, err)
fyrchik marked this conversation as resolved Outdated

require.NoError(t, err)?

`require.NoError(t, err)`?

Fixed

Fixed
hasResult, err := hasResult(ctrDI.DebugInfo, "PutNumber")
require.NoError(t, err)
someNum := getNumToPut()
fyrchik marked this conversation as resolved Outdated

same here

same here

Fixed

Fixed
// set up a VM for covertest.Run()
covertestRunVM := setUpVMForPut(t, e, ctrDI.Contract, hasResult, startOffsetPutNumber, someNum, invalidKey)
fyrchik marked this conversation as resolved Outdated

Comment should have a space after // (this is not a rule per se, but being uniform is a nice quality for a codebase). See also https://tip.golang.org/doc/comment

Comment should have a space after `// ` (this is not a rule per se, but being uniform is a nice quality for a codebase). See also https://tip.golang.org/doc/comment

Fixed

Fixed
res, covErr := covertest.Run(covertestRunVM)
t.Log("Printing collected instructions:")
spew.Dump(res)
t.Log("covertest.Run() returned an error: ", covErr)
fyrchik marked this conversation as resolved Outdated

For this case you might want to look at https://pkg.go.dev/github.com/stretchr/testify/require package.
We want the test to fail, not just print info.

Example of usage:

For this case you might want to look at https://pkg.go.dev/github.com/stretchr/testify/require package. We want the test to _fail_, not just print info. Example of usage: https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/src/commit/3dc8129ed79406b7f0e523be471f30e3014b9107/object/attribute_test.go#L17

Fixed

Fixed
// set up a VM for vm.Run()
origRunVM := setUpVMForPut(t, e, ctrDI.Contract, hasResult, startOffsetPutNumber, someNum, invalidKey)
runerr := origRunVM.Run()
t.Log("vm.Run() returned an error: ", covErr)
// check if errors are the same
require.Equal(t, runerr.Error(), covErr.Error())
// check if the number of elements on the stack is the same
require.Equal(t, origRunVM.Estack().Len(), covertestRunVM.Estack().Len())
}
func setUpVMForPut(t *testing.T, e *neotest.Executor, contract *neotest.Contract, hasResult bool, methodOff int, num int, key []byte) (v *vm.VM) {
ic, err := e.Chain.GetTestVM(trigger.Application, nil, nil)
require.NoError(t, err)
ic.VM.LoadNEFMethod(contract.NEF, contract.Hash, contract.Hash, callflag.All, hasResult, methodOff, -1, nil)
ic.VM.Context().Estack().PushVal(num)
ic.VM.Context().Estack().PushVal(key)
return ic.VM
}
func getStartOffset(di *compiler.DebugInfo, methodID string) (int, error) {
for _, method := range di.Methods {
if method.ID == methodID {
return int(method.Range.Start), nil
}
}
return 0, errors.New("Method not found")
}
func hasResult(di *compiler.DebugInfo, methodID string) (bool, error) {
for _, method := range di.Methods {
if method.ID == methodID {
if method.ReturnType == "Void" {
return false, nil
}
return true, nil
}
}
return false, errors.New("Method not found")
}
func getNumToPut() int {
return rand.Intn(100)
}