Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement action test suite helper #1139

Merged
merged 41 commits into from
Jul 23, 2024
Merged

Implement action test suite helper #1139

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
9e53841
implement action test suite helper
iFrostizz Jul 15, 2024
fb5d9a7
add morpheusvm action tests
iFrostizz Jul 15, 2024
18ffdf2
add check for contains error for dynamic errors
iFrostizz Jul 15, 2024
f189b56
remove morpheusvm test, simplify suite
iFrostizz Jul 15, 2024
504df82
add morpheusvm parametrized tests
iFrostizz Jul 15, 2024
c07bca0
Merge branch 'main' into action_testing_suite
iFrostizz Jul 15, 2024
adcf1ae
add licence
iFrostizz Jul 15, 2024
48af784
Merge branch 'main' into action_testing_suite
iFrostizz Jul 15, 2024
566fff4
check err
iFrostizz Jul 15, 2024
4f860f1
Merge branch 'action_testing_suite' of github.com:ava-labs/hypersdk i…
iFrostizz Jul 15, 2024
96a8d0a
cleanup
iFrostizz Jul 15, 2024
900cf09
remove `Actor` usage and implement `InMemoryStore`
iFrostizz Jul 16, 2024
41c7e18
Merge branch 'main' into action_testing_suite
iFrostizz Jul 16, 2024
85b254b
remove unused ctx
iFrostizz Jul 16, 2024
5a11a97
add review suggestions
iFrostizz Jul 16, 2024
5fc6d05
Update chaintesting/action_test_helpers.go
iFrostizz Jul 17, 2024
5dac07b
Update chaintesting/action_test_helpers.go
iFrostizz Jul 17, 2024
25f23e2
add tests, set actor, add assertion
iFrostizz Jul 17, 2024
94c5cf0
Merge branch 'action_testing_suite' of github.com:ava-labs/hypersdk i…
iFrostizz Jul 17, 2024
0fe76f1
Updated Token Example (#1142)
samliok Jul 16, 2024
8e5bc5b
Expose state functions from context (#1153)
richardpringle Jul 17, 2024
5b5ff1c
use `WindowSize` constant (#1155)
iFrostizz Jul 17, 2024
c31613d
update readme to point to an existing tokenvm action (#1049)
najeal Jul 17, 2024
5197db3
Refactor `vm.Config` out of `vm.Controller` (#1146)
joshua-kim Jul 17, 2024
2bb3973
remove unused parallelism key from test configs (#1158)
joshua-kim Jul 17, 2024
b3ed3b5
Remove `key_create` endpoint and use `Address` everywhere (#1121)
iFrostizz Jul 17, 2024
dbc4076
rename to chaintest, slice of `ActionTest`, remove suite
iFrostizz Jul 18, 2024
45383c3
Merge branch 'main' into action_testing_suite
iFrostizz Jul 18, 2024
8cd0738
rename to `InMemoryStore` to export type
iFrostizz Jul 18, 2024
c9c9dea
Merge branch 'main' into action_testing_suite
iFrostizz Jul 18, 2024
160137b
Update examples/morpheusvm/actions/transfer_test.go
iFrostizz Jul 18, 2024
b2be701
add sender post-balance assertion
iFrostizz Jul 18, 2024
3c266e6
Merge branch 'main' into action_testing_suite
iFrostizz Jul 19, 2024
9055d39
pass in `*testing.T` to the `Assertion` func pointer
iFrostizz Jul 19, 2024
7b55c6f
Merge branch 'action_testing_suite' of github.com:ava-labs/hypersdk i…
iFrostizz Jul 19, 2024
86f8f46
pass context from `Run`, handle error signal on done
iFrostizz Jul 19, 2024
ddd5084
Context plumbing in `ActionTest` suite (#1170)
ARR4N Jul 22, 2024
658b810
write helper function
iFrostizz Jul 22, 2024
0cf9190
Update chaintest/action_test_helpers.go
iFrostizz Jul 23, 2024
c97b9b9
Update examples/morpheusvm/actions/transfer_test.go
iFrostizz Jul 23, 2024
0286e57
Merge branch 'main' into action_testing_suite
iFrostizz Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions chain/action_test_helpers.go
View file
Open in desktop
iFrostizz marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Copyright (C) 2024, Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.

package chain

import (
"context"
"testing"

"github.com/ava-labs/avalanchego/ids"
"github.com/stretchr/testify/require"

"github.com/ava-labs/hypersdk/codec"
"github.com/ava-labs/hypersdk/state"
)

type ActionTest struct {
Action Action

Rules Rules
State state.Mutable
Timestamp int64
Actor codec.Address
ActionID ids.ID

ExpectedOutputs [][]byte
ExpectedErr error
}

type ActionTestSuite struct {
Tests map[string]ActionTest
}

func (suite *ActionTestSuite) Run(t *testing.T) {
for testName := range suite.Tests {
iFrostizz marked this conversation as resolved.
Show resolved Hide resolved
t.Run(testName, func(t *testing.T) {
require := require.New(t)
test := suite.Tests[testName]
iFrostizz marked this conversation as resolved.
Show resolved Hide resolved

output, err := test.Action.Execute(context.TODO(), test.Rules, test.State, test.Timestamp, test.Actor, test.ActionID)

require.ErrorIs(err, test.ExpectedErr)
require.Equal(output, test.ExpectedOutputs)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err and output should be mutually exclusive but it doesn't hurt to still check it at the end, even if err != nil

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now using errors.Is

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You won't reach the output check because you're using require; swapping to assert will use t.Errorf() under the hood instead of t.Fatalf(), but I don't think you should.

it doesn't hurt to still check it at the end, even if err != nil

I think it does actually, even if very minor, because it forces every error path to have a strict return value (presumably nil) when in reality the value is undefined.

More context should anyone be interested.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long live a healthy combination of assert and require

Copy link
Contributor

@darioush darioush Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep using require, otherwise the test author needs to keep thinking about which failures will cause the test to fail vs where the test can continue.

If the test fails, then debugging is needed and often the first point of unexpected behavior is what is most useful for debugging.

Additionally, require is more concise than if err != nil style checks, which is needed to use t.Fatal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep using require, otherwise the test author needs to keep thinking about which failures will cause the test to fail vs where the test can continue.

That's actually the point. It avoids whack-a-mole fixes where you get partial information about problems. The test author really should be thinking about that because they're the one person with the greatest insight.

My rule of thumb is that errors use require because that almost certainly guarantees that all other returned values are incorrect. Incorrect actual or got values, however, are typically more suited to assert.

Additionally, require is more concise than if err != nil style checks, which is needed to use t.Fatal.

require is equivalent to t.Fatal so there's no need for the err != nil check. The alternative is assert, which is equivalent to t.Error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would we know that we have a problem of partial information? I haven't heard folks complaining about it, but maybe we're just inured...

I think that in most cases the use of testify with its rich set of assertions (and correspondingly useful contextual output on failure) obviates most of the concerns expressed in the style guide. Where those assertions aren't sufficient, we're always free to implement custom checks.

If we did decide to mix assert and require, there are cases where a group of assertion calls would need a subsequent check to ensure that any of the assertion calls failing would subsequently fail the test e.g.

mySlice, err := myFunc(...)
require.Error(err) // No point in checking the value on error
// bunch of assert.* calls
// fail the test if any of the previous assert.* calls failed

// Subsequent calls that depended on the correctness of mySlice

})
}
}
80 changes: 80 additions & 0 deletions examples/morpheusvm/actions/transfer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
// Copyright (C) 2023, Ava Labs, Inc. All rights reserved.
// See the file LICENSE for licensing terms.

package actions

import (
"context"
"encoding/binary"
"testing"

"github.com/stretchr/testify/require"

"github.com/ava-labs/hypersdk/chain"
"github.com/ava-labs/hypersdk/codec"
"github.com/ava-labs/hypersdk/examples/morpheusvm/storage"
"github.com/ava-labs/hypersdk/state"
"github.com/ava-labs/hypersdk/tstate"
)

func TestTransferAction(t *testing.T) {
require := require.New(t)
ts := tstate.New(1)

tests := map[string]chain.ActionTest{
"ZeroTransfer": {
Action: &Transfer{
To: codec.EmptyAddress,
Value: 0,
},
ExpectedErr: ErrOutputValueZero,
},
iFrostizz marked this conversation as resolved.
Show resolved Hide resolved
"InvalidStateKey": {
Action: &Transfer{
To: codec.EmptyAddress,
Value: 1,
},
State: ts.NewView(map[string]state.Permissions{}, map[string][]byte{}),
iFrostizz marked this conversation as resolved.
Show resolved Hide resolved
ExpectedErr: tstate.ErrInvalidKeyOrPermission,
},
"NotEnoughBalance": {
iFrostizz marked this conversation as resolved.
Show resolved Hide resolved
Action: &Transfer{
To: codec.EmptyAddress,
Value: 1,
},
Actor: codec.EmptyAddress,
iFrostizz marked this conversation as resolved.
Show resolved Hide resolved
State: func() state.Mutable {
keys := make(state.Keys)
k := storage.BalanceKey(codec.EmptyAddress)
keys.Add(string(k), state.Read)
tsv := ts.NewView(keys, map[string][]byte{})
return tsv
}(),
ExpectedErr: storage.ErrInvalidBalance,
},
"SimpleTransfer": {
iFrostizz marked this conversation as resolved.
Show resolved Hide resolved
Action: &Transfer{
To: codec.EmptyAddress,
Value: 1,
},
Actor: codec.EmptyAddress,
State: func() state.Mutable {
keys := make(state.Keys)
k := storage.BalanceKey(codec.EmptyAddress)
keys.Add(string(k), state.All)
stor := map[string][]byte{}
tsv := ts.NewView(keys, stor)
b := make([]byte, 8)
binary.LittleEndian.PutUint64(b, uint64(1))
require.NoError(tsv.Insert(context.TODO(), k, b))
marun marked this conversation as resolved.
Show resolved Hide resolved
return tsv
}(),
},
}

testSuite := chain.ActionTestSuite{
Tests: tests,
}

testSuite.Run(t)
}
1 change: 0 additions & 1 deletion examples/tokenvm/storage/storage.go
View file
Open in desktop
iFrostizz marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"github.com/ava-labs/hypersdk/state"

smath "github.com/ava-labs/avalanchego/utils/math"

iFrostizz marked this conversation as resolved.
Show resolved Hide resolved
tconsts "github.com/ava-labs/hypersdk/examples/tokenvm/consts"
)

Expand Down
2 changes: 1 addition & 1 deletion examples/tokenvm/tests/integration/integration_test.go
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as https://github.com/ava-labs/hypersdk/pull/1139/files#r1679210787

Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/stretchr/testify/require"
"go.uber.org/zap"

"github.com/ava-labs/hypersdk/auth"
"github.com/ava-labs/hypersdk/chain"
"github.com/ava-labs/hypersdk/codec"
"github.com/ava-labs/hypersdk/consts"
Expand All @@ -42,7 +43,6 @@ import (
"github.com/ava-labs/hypersdk/rpc"
"github.com/ava-labs/hypersdk/vm"

"github.com/ava-labs/hypersdk/auth"
tconsts "github.com/ava-labs/hypersdk/examples/tokenvm/consts"
trpc "github.com/ava-labs/hypersdk/examples/tokenvm/rpc"
hutils "github.com/ava-labs/hypersdk/utils"
Expand Down
Loading