Add a custom vm.Run() function #9

Merged
fyrchik merged 1 commit from elebedeva/contract-coverage-primer:covertest/run into master 2024-09-04 19:51:22 +00:00
Member

Close #2

Adding covertest.Run() function which executes loaded program and accumulates all seen opcodes together with the scripthash they belong to.

Close #2 Adding `covertest.Run()` function which executes loaded program and accumulates all seen opcodes together with the scripthash they belong to.
elebedeva added 2 commits 2023-08-04 09:46:51 +00:00
It executes loaded program and accumulates all seen opcodes
together with the scripthash they belong to.

Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>
Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>
elebedeva requested review from fyrchik 2023-08-04 09:47:05 +00:00
fyrchik reviewed 2023-08-04 11:00:12 +00:00
covertest/run.go Outdated
@ -0,0 +9,4 @@
"github.com/nspcc-dev/neo-go/pkg/vm/vmstate"
)
// InstrHash maps Instruction with its Script Hash.
Owner

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.
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
covertest/run.go Outdated
@ -0,0 +11,4 @@
// InstrHash maps Instruction with its Script Hash.
type InstrHash struct {
Num int
Owner

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)
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
covertest/run.go Outdated
@ -0,0 +18,4 @@
// Run starts execution of the loaded program and accumulates all seen opcodes
// together with the scripthash they belong to.
func Run(v *vm.VM) ([]InstrHash, error) {
Owner

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.
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -0,0 +19,4 @@
// Run starts execution of the loaded program and accumulates all seen opcodes
// together with the scripthash they belong to.
func Run(v *vm.VM) ([]InstrHash, error) {
Owner

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)
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -44,0 +53,4 @@
e.DeployContract(t, ctrDI.Contract, nil)
// setting up a VM for covertest.Run()
covertestRunVM := setUpVMForPut(t, e, ctrDI.Contract, false, 101, 2, invalidKey)
Owner

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

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

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -44,0 +55,4 @@
// setting up a VM for covertest.Run()
covertestRunVM := setUpVMForPut(t, e, ctrDI.Contract, false, 101, 2, invalidKey)
res, err := covertest.Run(covertestRunVM)
spew.Println("Printing collected instructions:")
Owner

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.
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -44,0 +64,4 @@
runerr := origRunVM.Run()
spew.Println("vm.Run() returned an error: ", err)
//check if errors are the same
Owner

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
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -44,0 +68,4 @@
spew.Println("Are errors the same? ", runerr.Error() == runerr.Error())
//check if the number of elements on the stack is the same
spew.Println("Is the number of elements on the stack the same? ", origRunVM.Estack().Len() == covertestRunVM.Estack().Len())
Owner

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
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
elebedeva added 1 commit 2023-08-07 08:58:11 +00:00
Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>
fyrchik approved these changes 2023-08-08 08:18:16 +00:00
fyrchik reviewed 2023-08-08 08:19:47 +00:00
@ -44,0 +56,4 @@
e.DeployContract(t, ctrDI.Contract, nil)
startOffsetPutNumber, err := getStartOffset(ctrDI.DebugInfo, "PutNumber")
if err != nil {
Owner

require.NoError(t, err)?

`require.NoError(t, err)`?
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -44,0 +62,4 @@
}
hasResult, err := hasResult(ctrDI.DebugInfo, "PutNumber")
if err != nil {
Owner

same here

same here
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
fyrchik requested review from fyrchik 2023-08-08 08:20:14 +00:00
elebedeva force-pushed covertest/run from d3b48812bb to 4d48eff4c0 2023-08-08 08:52:15 +00:00 Compare
elebedeva force-pushed covertest/run from 4d48eff4c0 to 401d54dda4 2023-08-08 10:44:29 +00:00 Compare
fyrchik approved these changes 2023-08-08 11:16:18 +00:00
fyrchik merged commit 401d54dda4 into master 2023-08-08 11:16:24 +00:00
elebedeva deleted branch covertest/run 2023-08-08 12:05:19 +00:00
Sign in to join this conversation.
No description provided.