From 71a196cfef947dc61ea6acde9b587e0629e619d7 Mon Sep 17 00:00:00 2001 From: David Porter Date: Tue, 5 Mar 2024 23:39:15 -0800 Subject: [PATCH] Straightforwardly fixes a few minor copy bugs and adds a small fuzz util (#5572) - Adds the library github.com/google/gofuzz. - Adds a small wrapper for some repo-level conventions we may wish to follow by default - notably determinitism and logging the seed value - Adds some probably fairly low value but still easy to test copy functions as a demonstration of value. --- cmd/server/go.sum | 1 + common/testing/testdatagen/fuzzer.go | 38 ++++++++++ .../testdatagen/idlfuzzedtestdata/history.go | 65 ++++++++++++++++ go.mod | 2 + go.sum | 1 + .../engine/engineimpl/historyEngine2_test.go | 2 + .../historyEngine3_eventsv2_test.go | 2 +- .../history/execution/mutable_state_util.go | 31 ++++---- .../execution/mutable_state_util_test.go | 74 +++++++++++++++++++ 9 files changed, 201 insertions(+), 15 deletions(-) create mode 100644 common/testing/testdatagen/fuzzer.go create mode 100644 common/testing/testdatagen/idlfuzzedtestdata/history.go diff --git a/cmd/server/go.sum b/cmd/server/go.sum index 9740f4bce81..c6d604ed49d 100644 --- a/cmd/server/go.sum +++ b/cmd/server/go.sum @@ -187,6 +187,7 @@ github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/go-cmp v0.5.7/go.mod h1:n+brtR0CgQNWTVd5ZUFpTBC8YFBDLK/h/bpaJ8/DtOE= github.com/google/go-cmp v0.5.8/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= +github.com/google/gofuzz v1.0.0 h1:A8PeW59pxE9IoFRqBp37U+mSNaQoZ46F1f0f863XSXw= github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/martian/v3 v3.3.2 h1:IqNFLAmvJOgVlpdEBiQbDc2EwKW77amAycfTuWKdfvw= github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI= diff --git a/common/testing/testdatagen/fuzzer.go b/common/testing/testdatagen/fuzzer.go new file mode 100644 index 00000000000..e9fcef23fb0 --- /dev/null +++ b/common/testing/testdatagen/fuzzer.go @@ -0,0 +1,38 @@ +// The MIT License (MIT) + +// Copyright (c) 2017-2020 Uber Technologies Inc. + +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package testdatagen + +import ( + "testing" + "time" + + fuzz "github.com/google/gofuzz" +) + +// NewFuzzer creates a new fuzzer, notes down the deterministic seed +func New(t *testing.T, generatorFuncs ...interface{}) *fuzz.Fuzzer { + seed := time.Now().Unix() + t.Log("Fuzz Seed:", seed) + + return fuzz.NewWithSeed(time.Now().Unix()).Funcs(generatorFuncs...).NilChance(0.2) +} diff --git a/common/testing/testdatagen/idlfuzzedtestdata/history.go b/common/testing/testdatagen/idlfuzzedtestdata/history.go new file mode 100644 index 00000000000..c7e2d48c736 --- /dev/null +++ b/common/testing/testdatagen/idlfuzzedtestdata/history.go @@ -0,0 +1,65 @@ +// The MIT License (MIT) + +// Copyright (c) 2017-2020 Uber Technologies Inc. + +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package idlfuzzedtestdata + +import ( + "testing" + + fuzz "github.com/google/gofuzz" + + "github.com/uber/cadence/common/testing/testdatagen" + "github.com/uber/cadence/common/types" + "github.com/uber/cadence/common/types/testdata" +) + +// NewFuzzerWithIDLTypes creates a new fuzzer, notes down the deterministic seed +// this particular invocation is preconfigured to be able to handle idl structs +// correctly without generating completely invalid data (which, while good to test for +// in the context of an application is too wide a search to be useful) +func NewFuzzerWithIDLTypes(t *testing.T) *fuzz.Fuzzer { + return testdatagen.New(t, + // USE THESE VERY SPARINGLY, ONLY WHEN YOU MUST! + // + // The goal of providing these generators for specific types should be + // to use them as little as possible, as they are fixed test data + // which will not evolve with the idl or functions, therefore + // the main benefit of fuzzing - evolving tests to handle all new fields in place - + // will be defeated. + // + // for example, for mappers, if you add a new field that needs to be + // mapped from protobuf to a native-go type (from the types folder) + // and the testdata is fixed here *and not updated*, then the issue + // will not be caught by any roundtrip tests. + GenHistoryEvent, + ) +} + +// GenHistoryEvent is a function to use with gofuzz which +// skips the majority of difficult to generate values +// for the sake of simplicity in testing. Use it with the fuzz.Funcs(...) generation function +func GenHistoryEvent(o *types.HistoryEvent, c fuzz.Continue) { + // todo (david.porter) setup an assertion to ensure this list is exhaustive + i := c.Rand.Intn(len(testdata.HistoryEventArray) - 1) + o = testdata.HistoryEventArray[i] + return +} diff --git a/go.mod b/go.mod index 7bdb937a621..487f6339943 100644 --- a/go.mod +++ b/go.mod @@ -67,6 +67,8 @@ require ( gopkg.in/yaml.v2 v2.3.0 ) +require github.com/google/gofuzz v1.0.0 + require ( github.com/BurntSushi/toml v0.4.1 // indirect github.com/anmitsu/go-shlex v0.0.0-20161002113705-648efa622239 // indirect diff --git a/go.sum b/go.sum index 3d398136a89..2e5d16db790 100644 --- a/go.sum +++ b/go.sum @@ -203,6 +203,7 @@ github.com/google/go-cmp v0.5.7/go.mod h1:n+brtR0CgQNWTVd5ZUFpTBC8YFBDLK/h/bpaJ8 github.com/google/go-cmp v0.5.8/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +github.com/google/gofuzz v1.0.0 h1:A8PeW59pxE9IoFRqBp37U+mSNaQoZ46F1f0f863XSXw= github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/pprof v0.0.0-20181127221834-b4f47329b966/go.mod h1:zfwlbNMJ+OItoe0UupaVj+oy1omPYYDuagoSzA8v9mc= github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI= diff --git a/service/history/engine/engineimpl/historyEngine2_test.go b/service/history/engine/engineimpl/historyEngine2_test.go index d279fba8857..bd3c2e931f7 100644 --- a/service/history/engine/engineimpl/historyEngine2_test.go +++ b/service/history/engine/engineimpl/historyEngine2_test.go @@ -224,6 +224,7 @@ func (s *engine2Suite) TestRecordDecisionTaskStartedSuccessStickyExpired() { s.NotNil(response) expectedResponse.StartedTimestamp = response.StartedTimestamp expectedResponse.ScheduledTimestamp = common.Int64Ptr(0) + response.ScheduledTimestamp = common.Int64Ptr(0) expectedResponse.Queries = make(map[string]*types.WorkflowQuery) s.Equal(&expectedResponse, response) } @@ -299,6 +300,7 @@ func (s *engine2Suite) TestRecordDecisionTaskStartedSuccessStickyEnabled() { s.NotNil(response) expectedResponse.StartedTimestamp = response.StartedTimestamp expectedResponse.ScheduledTimestamp = common.Int64Ptr(0) + response.ScheduledTimestamp = common.Int64Ptr(0) expectedResponse.Queries = make(map[string]*types.WorkflowQuery) s.Equal(&expectedResponse, response) } diff --git a/service/history/engine/engineimpl/historyEngine3_eventsv2_test.go b/service/history/engine/engineimpl/historyEngine3_eventsv2_test.go index ebce68c5349..363172224c2 100644 --- a/service/history/engine/engineimpl/historyEngine3_eventsv2_test.go +++ b/service/history/engine/engineimpl/historyEngine3_eventsv2_test.go @@ -217,7 +217,7 @@ func (s *engine3Suite) TestRecordDecisionTaskStartedSuccessStickyEnabled() { s.Nil(err) s.NotNil(response) expectedResponse.StartedTimestamp = response.StartedTimestamp - expectedResponse.ScheduledTimestamp = common.Int64Ptr(0) + expectedResponse.ScheduledTimestamp = response.ScheduledTimestamp expectedResponse.Queries = make(map[string]*types.WorkflowQuery) s.Equal(&expectedResponse, response) } diff --git a/service/history/execution/mutable_state_util.go b/service/history/execution/mutable_state_util.go index 691b7208748..6d75bf825c2 100644 --- a/service/history/execution/mutable_state_util.go +++ b/service/history/execution/mutable_state_util.go @@ -24,6 +24,8 @@ package execution import ( "encoding/json" + "golang.org/x/exp/slices" + "github.com/uber/cadence/common/cache" "github.com/uber/cadence/common/clock" "github.com/uber/cadence/common/persistence" @@ -328,6 +330,7 @@ func CopyWorkflowExecutionInfo(sourceInfo *persistence.WorkflowExecutionInfo) *p ParentDomainID: sourceInfo.ParentDomainID, ParentWorkflowID: sourceInfo.ParentWorkflowID, ParentRunID: sourceInfo.ParentRunID, + IsCron: sourceInfo.IsCron, InitiatedID: sourceInfo.InitiatedID, CompletionEventBatchID: sourceInfo.CompletionEventBatchID, CompletionEvent: sourceInfo.CompletionEvent, @@ -354,6 +357,7 @@ func CopyWorkflowExecutionInfo(sourceInfo *persistence.WorkflowExecutionInfo) *p DecisionRequestID: sourceInfo.DecisionRequestID, DecisionTimeout: sourceInfo.DecisionTimeout, DecisionAttempt: sourceInfo.DecisionAttempt, + DecisionScheduledTimestamp: sourceInfo.DecisionScheduledTimestamp, DecisionStartedTimestamp: sourceInfo.DecisionStartedTimestamp, DecisionOriginalScheduledTimestamp: sourceInfo.DecisionOriginalScheduledTimestamp, CancelRequested: sourceInfo.CancelRequested, @@ -381,8 +385,7 @@ func CopyWorkflowExecutionInfo(sourceInfo *persistence.WorkflowExecutionInfo) *p // CopyActivityInfo copies ActivityInfo func CopyActivityInfo(sourceInfo *persistence.ActivityInfo) *persistence.ActivityInfo { - details := make([]byte, len(sourceInfo.Details)) - copy(details, sourceInfo.Details) + details := slices.Clone(sourceInfo.Details) return &persistence.ActivityInfo{ Version: sourceInfo.Version, @@ -437,24 +440,24 @@ func CopyTimerInfo(sourceInfo *persistence.TimerInfo) *persistence.TimerInfo { // CopyCancellationInfo copies RequestCancelInfo func CopyCancellationInfo(sourceInfo *persistence.RequestCancelInfo) *persistence.RequestCancelInfo { return &persistence.RequestCancelInfo{ - Version: sourceInfo.Version, - InitiatedID: sourceInfo.InitiatedID, - CancelRequestID: sourceInfo.CancelRequestID, + Version: sourceInfo.Version, + InitiatedID: sourceInfo.InitiatedID, + InitiatedEventBatchID: sourceInfo.InitiatedEventBatchID, + CancelRequestID: sourceInfo.CancelRequestID, } } // CopySignalInfo copies SignalInfo func CopySignalInfo(sourceInfo *persistence.SignalInfo) *persistence.SignalInfo { result := &persistence.SignalInfo{ - Version: sourceInfo.Version, - InitiatedID: sourceInfo.InitiatedID, - SignalRequestID: sourceInfo.SignalRequestID, - SignalName: sourceInfo.SignalName, - } - result.Input = make([]byte, len(sourceInfo.Input)) - copy(result.Input, sourceInfo.Input) - result.Control = make([]byte, len(sourceInfo.Control)) - copy(result.Control, sourceInfo.Control) + Version: sourceInfo.Version, + InitiatedEventBatchID: sourceInfo.InitiatedEventBatchID, + InitiatedID: sourceInfo.InitiatedID, + SignalRequestID: sourceInfo.SignalRequestID, + SignalName: sourceInfo.SignalName, + } + result.Input = slices.Clone(sourceInfo.Input) + result.Control = slices.Clone(sourceInfo.Control) return result } diff --git a/service/history/execution/mutable_state_util_test.go b/service/history/execution/mutable_state_util_test.go index 3b412654ba6..030062fb97f 100644 --- a/service/history/execution/mutable_state_util_test.go +++ b/service/history/execution/mutable_state_util_test.go @@ -28,9 +28,83 @@ import ( "github.com/uber/cadence/common" "github.com/uber/cadence/common/clock" + "github.com/uber/cadence/common/persistence" + "github.com/uber/cadence/common/testing/testdatagen/idlfuzzedtestdata" "github.com/uber/cadence/common/types" ) +func TestCopyActivityInfo(t *testing.T) { + t.Run("test CopyActivityInfo Mapping", func(t *testing.T) { + f := idlfuzzedtestdata.NewFuzzerWithIDLTypes(t) + + d1 := persistence.ActivityInfo{} + f.Fuzz(&d1) + d2 := CopyActivityInfo(&d1) + + assert.Equal(t, &d1, d2) + }) +} + +func TestCopyWorkflowExecutionInfo(t *testing.T) { + t.Run("test ExecutionInfo Mapping", func(t *testing.T) { + f := idlfuzzedtestdata.NewFuzzerWithIDLTypes(t) + + d1 := persistence.WorkflowExecutionInfo{} + f.Fuzz(&d1) + d2 := CopyWorkflowExecutionInfo(&d1) + + assert.Equal(t, &d1, d2) + }) +} + +func TestCopyTimerInfoMapping(t *testing.T) { + t.Run("test Timer info Mapping", func(t *testing.T) { + f := idlfuzzedtestdata.NewFuzzerWithIDLTypes(t) + + d1 := persistence.TimerInfo{} + f.Fuzz(&d1) + d2 := CopyTimerInfo(&d1) + + assert.Equal(t, &d1, d2) + }) +} + +func TestChildWorkflowMapping(t *testing.T) { + t.Run("test child workflwo info Mapping", func(t *testing.T) { + f := idlfuzzedtestdata.NewFuzzerWithIDLTypes(t) + + d1 := persistence.ChildExecutionInfo{} + f.Fuzz(&d1) + d2 := CopyChildInfo(&d1) + + assert.Equal(t, &d1, d2) + }) +} + +func TestCopySignalInfo(t *testing.T) { + t.Run("test signal info Mapping", func(t *testing.T) { + f := idlfuzzedtestdata.NewFuzzerWithIDLTypes(t) + + d1 := persistence.SignalInfo{} + f.Fuzz(&d1) + d2 := CopySignalInfo(&d1) + + assert.Equal(t, &d1, d2) + }) +} + +func TestCopyCancellationInfo(t *testing.T) { + t.Run("test signal info Mapping", func(t *testing.T) { + f := idlfuzzedtestdata.NewFuzzerWithIDLTypes(t) + + d1 := persistence.RequestCancelInfo{} + f.Fuzz(&d1) + d2 := CopyCancellationInfo(&d1) + + assert.Equal(t, &d1, d2) + }) +} + func TestFindAutoResetPoint(t *testing.T) { timeSource := clock.NewRealTimeSource()