From 2d5d663f06d313340b1043686ea72a5355767ee6 Mon Sep 17 00:00:00 2001 From: meows Date: Tue, 6 Sep 2022 15:03:11 -0700 Subject: [PATCH 1/3] tests: add error check for false negatives Some tests define an 'expectException' error but the tests runner does not check for conditions where this test value is filled (error expected) but in which no error is returned by the test runner. An example of this scenario is GeneralStateTests/stTransactionTest/HighGasPrice.json, which expects a 'TR_NoFunds' error, but the test runner does not return any error. Date: 2022-09-06 15:03:11-07:00 Signed-off-by: meows --- tests/state_test.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/state_test.go b/tests/state_test.go index d33ebc4b00db..74d4dbc9574d 100644 --- a/tests/state_test.go +++ b/tests/state_test.go @@ -57,12 +57,12 @@ func TestState(t *testing.T) { // Broken tests: // Expected failures: - //st.fails(`^stRevertTest/RevertPrecompiledTouch(_storage)?\.json/Byzantium/0`, "bug in test") - //st.fails(`^stRevertTest/RevertPrecompiledTouch(_storage)?\.json/Byzantium/3`, "bug in test") - //st.fails(`^stRevertTest/RevertPrecompiledTouch(_storage)?\.json/Constantinople/0`, "bug in test") - //st.fails(`^stRevertTest/RevertPrecompiledTouch(_storage)?\.json/Constantinople/3`, "bug in test") - //st.fails(`^stRevertTest/RevertPrecompiledTouch(_storage)?\.json/ConstantinopleFix/0`, "bug in test") - //st.fails(`^stRevertTest/RevertPrecompiledTouch(_storage)?\.json/ConstantinopleFix/3`, "bug in test") + // st.fails(`^stRevertTest/RevertPrecompiledTouch(_storage)?\.json/Byzantium/0`, "bug in test") + // st.fails(`^stRevertTest/RevertPrecompiledTouch(_storage)?\.json/Byzantium/3`, "bug in test") + // st.fails(`^stRevertTest/RevertPrecompiledTouch(_storage)?\.json/Constantinople/0`, "bug in test") + // st.fails(`^stRevertTest/RevertPrecompiledTouch(_storage)?\.json/Constantinople/3`, "bug in test") + // st.fails(`^stRevertTest/RevertPrecompiledTouch(_storage)?\.json/ConstantinopleFix/0`, "bug in test") + // st.fails(`^stRevertTest/RevertPrecompiledTouch(_storage)?\.json/ConstantinopleFix/3`, "bug in test") // For Istanbul, older tests were moved into LegacyTests for _, dir := range []string{ @@ -82,6 +82,9 @@ func TestState(t *testing.T) { // Ignore expected errors (TODO MariusVanDerWijden check error string) return nil } + if err == nil && len(test.json.Post[subtest.Fork][subtest.Index].ExpectException) > 0 { + return fmt.Errorf("expected exception %q", test.json.Post[subtest.Fork][subtest.Index].ExpectException) + } return st.checkFailure(t, err) }) }) @@ -97,6 +100,9 @@ func TestState(t *testing.T) { // Ignore expected errors (TODO MariusVanDerWijden check error string) return nil } + if err == nil && len(test.json.Post[subtest.Fork][subtest.Index].ExpectException) > 0 { + return fmt.Errorf("expected exception %q", test.json.Post[subtest.Fork][subtest.Index].ExpectException) + } return st.checkFailure(t, err) }) }) From a3ed8faf0a8238eaa24aec961081d3a2aaf1be89 Mon Sep 17 00:00:00 2001 From: meows Date: Tue, 6 Sep 2022 15:05:11 -0700 Subject: [PATCH 2/3] tests: raise error returned by core.ApplyMessage This allows the test runner to compare expected vs yielded errors. Date: 2022-09-06 15:05:11-07:00 Signed-off-by: meows --- tests/state_test_util.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/state_test_util.go b/tests/state_test_util.go index 38cdbc4d6504..4b3b6321cd8d 100644 --- a/tests/state_test_util.go +++ b/tests/state_test_util.go @@ -231,7 +231,8 @@ func (t *StateTest) RunNoVerify(subtest StateSubtest, vmconfig vm.Config, snapsh snapshot := statedb.Snapshot() gaspool := new(core.GasPool) gaspool.AddGas(block.GasLimit()) - if _, err := core.ApplyMessage(evm, msg, gaspool); err != nil { + _, err = core.ApplyMessage(evm, msg, gaspool) + if err != nil { statedb.RevertToSnapshot(snapshot) } // Add 0-value mining reward. This only makes a difference in the cases @@ -244,7 +245,7 @@ func (t *StateTest) RunNoVerify(subtest StateSubtest, vmconfig vm.Config, snapsh statedb.Commit(config.IsEIP158(block.Number())) // And _now_ get the state root root := statedb.IntermediateRoot(config.IsEIP158(block.Number())) - return snaps, statedb, root, nil + return snaps, statedb, root, err } func (t *StateTest) gasLimit(subtest StateSubtest) uint64 { From f8cbab54f5eff7b41b1ae7923814999f76a60b10 Mon Sep 17 00:00:00 2001 From: meows Date: Tue, 6 Sep 2022 15:34:31 -0700 Subject: [PATCH 3/3] tests: method checks error expectations for state tests Date: 2022-09-06 15:34:32-07:00 Signed-off-by: meows --- tests/state_test.go | 14 -------------- tests/state_test_util.go | 30 +++++++++++++++++++++++++++++- 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/tests/state_test.go b/tests/state_test.go index 74d4dbc9574d..cd287413bdc5 100644 --- a/tests/state_test.go +++ b/tests/state_test.go @@ -78,13 +78,6 @@ func TestState(t *testing.T) { t.Run(key+"/trie", func(t *testing.T) { withTrace(t, test.gasLimit(subtest), func(vmconfig vm.Config) error { _, _, err := test.Run(subtest, vmconfig, false) - if err != nil && len(test.json.Post[subtest.Fork][subtest.Index].ExpectException) > 0 { - // Ignore expected errors (TODO MariusVanDerWijden check error string) - return nil - } - if err == nil && len(test.json.Post[subtest.Fork][subtest.Index].ExpectException) > 0 { - return fmt.Errorf("expected exception %q", test.json.Post[subtest.Fork][subtest.Index].ExpectException) - } return st.checkFailure(t, err) }) }) @@ -96,13 +89,6 @@ func TestState(t *testing.T) { return err } } - if err != nil && len(test.json.Post[subtest.Fork][subtest.Index].ExpectException) > 0 { - // Ignore expected errors (TODO MariusVanDerWijden check error string) - return nil - } - if err == nil && len(test.json.Post[subtest.Fork][subtest.Index].ExpectException) > 0 { - return fmt.Errorf("expected exception %q", test.json.Post[subtest.Fork][subtest.Index].ExpectException) - } return st.checkFailure(t, err) }) }) diff --git a/tests/state_test_util.go b/tests/state_test_util.go index 4b3b6321cd8d..e5c8d2444942 100644 --- a/tests/state_test_util.go +++ b/tests/state_test_util.go @@ -159,11 +159,39 @@ func (t *StateTest) Subtests() []StateSubtest { return sub } +// checkError checks if the error returned by the state transition matches any expected error. +// A failing expectation returns a wrapped version of the original error, if any, +// or a new error detailing the failing expectation. +// This function does not return or modify the original error, it only evaluates and returns expectations for the error. +func (t *StateTest) checkError(subtest StateSubtest, err error) error { + expectedError := t.json.Post[subtest.Fork][subtest.Index].ExpectException + if err == nil && expectedError == "" { + return nil + } + if err == nil && expectedError != "" { + return fmt.Errorf("expected error %q, got no error", expectedError) + } + if err != nil && expectedError == "" { + return fmt.Errorf("unexpected error: %w", err) + } + if err != nil && expectedError != "" { + // Ignore expected errors (TODO MariusVanDerWijden check error string) + return nil + } + return nil +} + // Run executes a specific subtest and verifies the post-state and logs func (t *StateTest) Run(subtest StateSubtest, vmconfig vm.Config, snapshotter bool) (*snapshot.Tree, *state.StateDB, error) { snaps, statedb, root, err := t.RunNoVerify(subtest, vmconfig, snapshotter) + if checkedErr := t.checkError(subtest, err); checkedErr != nil { + return snaps, statedb, checkedErr + } + // The error has been checked; if it was unexpected, it's already returned. if err != nil { - return snaps, statedb, err + // Here, an error exists but it was expected. + // We do not check the post state or logs. + return snaps, statedb, nil } post := t.json.Post[subtest.Fork][subtest.Index] // N.B: We need to do this in a two-step process, because the first Commit takes care