offchain - unit testing improvements #117

dimkouv · 2023-09-06T18:38:18Z

Making the code more testable by replacing logpoller with two new ccipevents methods and customTokenPoolFactory plugin field.
Getting rid of a database connection in unit tests.
Re-writing several unit-tests.
Renaming and re-ordering.
Adding more unit tests.

mateusz-sekara · 2023-09-11T11:32:33Z

core/services/ocr2/plugins/ccip/execution_reporting_plugin.go

@@ -142,6 +141,9 @@ func (rf *ExecutionReportingPluginFactory) NewReportingPlugin(config types.Repor
 			offchainConfig:        offchainConfig,
 			cachedDestTokens:      cachedDestTokens,
 			cachedSourceFeeTokens: cachedSourceFeeTokens,
+			customTokenPoolFactory: func(ctx context.Context, contractBackend bind.ContractBackend, poolAddress common.Address) (custom_token_pool.CustomTokenPoolInterface, error) {


ctx is not used, probably can be removed from func

nit, I would consider flipping args order and making it consistent with the NewCustomTokenPool func, for instance

func(ctx context.Context, poolAddress common.Address, contractBackend bind.ContractBackend) (custom_token_pool.CustomTokenPoolInterface, error) { return custom_token_pool.NewCustomTokenPool(poolAddress, contractBackend) }

flipped the order, ctx might be useful in the future since contract client is also passed

core/services/ocr2/plugins/ccip/abihelpers/abi_helpers_test.go

mateusz-sekara · 2023-09-11T11:41:45Z

core/services/ocr2/plugins/ccip/commit_reporting_plugin_test.go

-}
-
-func setupCommitTestHarness(t *testing.T) commitTestHarness {
-	th := plugintesthelpers.SetupCCIPTestHarness(t)


It seems that func SetupCCIPTestHarness(t *testing.T) CCIPPluginTestHarness is no longer used and can be removed then

... and probably other functions from plugin_harness.go might become unused

mateusz-sekara · 2023-09-11T11:52:12Z

core/services/ocr2/plugins/ccip/commit_reporting_plugin_test.go

+		tokenPriceUpdates []ccipevents.Event[price_registry.PriceRegistryUsdPerTokenUpdated]
+		sendRequests      []ccipevents.Event[evm_2_evm_onramp.EVM2EVMOnRampCCIPSendRequested]
+
+		expCommitReport commit_store.CommitStoreCommitReport


nit, I am not sure what is the best way to do it in Go, but expCommitReport always contains a meaningful report when shouldGerReport is true. Meaning that maybe it would be easier to have it as a pointer and do the following:

testCases := []struct { expCommitReport *commit_store.CommitStoreCommitReport } if tc.expCommitReport != nil { encodedExpectedReport, err := abihelpers.EncodeCommitReport(tc.expCommitReport) assert.NoError(t, err) assert.Equal(t, types.Report(encodedExpectedReport), gotReport) }

yeah better

mateusz-sekara · 2023-09-11T11:56:38Z

core/services/ocr2/plugins/ccip/commit_reporting_plugin_test.go

-	lggr := logger.TestLogger(t)
-	priceGetter := newMockPriceGetter()
-
-	backendClient := client.NewSimulatedBackendClient(t, th.Dest.Chain, new(big.Int).SetUint64(th.Dest.ChainID))


Don't we want to keep some e2e tests using simulated backends anyway? I understand that mocking simplifies some things and makes tests more concise. However, we sacrifice e2e nature of those tests by mocking most of the deps (which may be fine). Anyway, it might be worth keeping some smokes for the simulated backend anyway to verify running on a mocked env, or maybe it's already covered somewhere else? WDYT?

IMO Everything should already be covered by the unit tests. A simulated backend, which is still 'fake/mock', adds complexity to the test suite and doesn't really cover/catch more cases. I believe that having real e2e integration tests and a proper unit tests suite is enough.

mateusz-sekara · 2023-09-11T11:58:11Z

core/services/ocr2/plugins/ccip/commit_reporting_plugin_test.go

+					Return(gas.EvmFee{Legacy: assets.NewWei(tc.fee)}, uint32(0), nil)
+			}
+
+			p := &CommitReportingPlugin{}


Why not init when declared?

p := &CommitReportingPlugin{ lggr: logger.TestLogger(t), inflightReports: newInflightCommitReportsContainer(time.Minute), ... }

just a matter of initialisation preference, imo not a big deal

IMO it's easier to read when a struct is initalized in one instruction instead of being splitted into multiple p.field = "x". When I see init split into multiple lines, I would assume that we need some extra computation steps between adding values to the fields. Up to you.

connorwstein · 2023-09-12T02:27:22Z

core/services/ocr2/plugins/ccip/ccipevents/client.go

@@ -22,6 +23,8 @@ type BlockMeta struct {
 }

 // Client can be used to fetch CCIP related parsed on-chain events.
+//
+//go:generate mockery --quiet --name Client --output . --filename mock.go --inpackage --case=underscore
 type Client interface {


wdyt about a rename of the pkg to ccipchain and this interface to Reader? Then plugins would make use of a ccipchain.Reader. More general than events since we make use of a LatestBlock and I can see that evolving into abstracting the evmclient/gas.EvmEstimator as well which will help us support non-evm

hmmm

reader.GetSendRequestsGteSeqNum reader.GetSendRequestsBetweenSeqNums reader.GetTokenPriceUpdatesCreatedAfter

Reader might cause confusion with the io.Reader

and I feel ccipchain not really related to what this package contains

Thinking out loud: This package contains functionality to easily access/fetch on-chain data/events related to ccip.
What about package name being: onchaindata and interface Fetcher?

onchaindata.Fetcher Plugin { sourceFetcher onchaindata.Fetcher destFetcher onchaindata.Fetcher }

I'm not worried about confusion with io.Reader with our own package prefix present. Fetcher/Accessor/Getter etc are all just less clear/work arounds to avoid Reader. I'm cool with onchaindata or even just onchain:

Plugin { source onchain.Reader dest onchain.Reader } source.GetSendRequestsGteSeqNum

connorwstein · 2023-09-12T02:37:03Z

core/services/ocr2/plugins/ccip/commit_reporting_plugin_test.go

-			assert.Equal(t, tt.expected, decodedObservation)
+			assert.NoError(t, err)
+
+			expObsBytes, _ := tc.expObs.Marshal()


assert no error?

lazy :D fixed but it would work anyway, test would fail.

connorwstein · 2023-09-12T03:17:09Z

core/services/ocr2/plugins/ccip/commit_reporting_plugin_test.go

+			commitStore := mock_contracts.NewCommitStoreInterface(t)
+			commitStore.On("IsUnpausedAndARMHealthy", mock.Anything).Return(!tc.commitStoreIsPaused, nil)
+			if tc.commitStoreSeqNum > 0 {
+				commitStore.On("GetExpectedNextSequenceNumber", mock.Anything).Return(tc.commitStoreSeqNum, nil)


All this conditional mocking makes it super tightly coupled to the impl and hard to read so refactoring will be very painful. I feel like a fake instead of a mock might significantly help us here. Some ideas:

Trim the interfaces to the bare minimum that we inject into the plugins. E.g. don't use EVM2EVMOffRampInterface/CommitStoreInterface etc. but define the smallest possible interface of only functions we use and inject that. That will make it much easier if we want a custom fake. Also related to my comment above regarding ccipchain.Reader, it'll help with the EVM abstraction as well (not relying on gethwrappers)

Introduce fakes such that we can construct the whole plugin in the state that we want for a given test case before we call some plugin function i.e. get as close as possible to this:

testCases := []struct { name string plugin CommitReportingPlugin expErr bool expObs CommitObservation }{ { name: "base report", plugin: CommitReportingPlugin{ config: CommitPluginConfig{ commitStore: NewFakeCommitPlugin(paused=false, nextSeqNr=1, epochAndRound=10), sourceEvents: NewFakeEvents(sendRequests=[a,b,c]) // TODO }, // TODO }, expObs: CommitObservation{ TokenPricesUSD: map[common.Address]*big.Int{ someTokenAddr: big.NewInt(20000000000), }, SourceGasPriceUSD: big.NewInt(0), Interval: commit_store.CommitStoreInterval{ Min: 54, Max: 55, }, }, },

We can introduce state modifications or errorOnCall params to the fakes if we need to as well, but should be able to get quite far with just static ones I'd imagine. Can probably still make use of mockery if you want too to minimize fake code i.e.:

func NewFakeCommitStore(bool disabled,...) CommitStore { m := mocks.CommitStore{} // Mockery generated for minimal CommitStore interface m.On("IsUnpausedAndARMHealthy", mock.Anything).Return(!disabled, nil).Maybe() // ... return m }

Another idea that comes to mind is breaking down the plugin steps into 2 distinct phases: state read and compute. Then unit test them separately e.g.:

(p Plugin) Observe(...) (Observation, error) obsState, err := p.ObservationState() // read all the relevant info from chain/caches/db etc. if err != nil { return nil, err } return computeObservation(obsState)

Yeah I agree, I will work a bit more to improve this

…ccip into ref/improve-test-suite

mateusz-sekara

lgtm

mateusz-sekara · 2023-09-18T09:39:36Z

core/services/ocr2/plugins/ccip/commit_reporting_plugin_test.go

+					Return(gas.EvmFee{Legacy: assets.NewWei(tc.fee)}, uint32(0), nil)
+			}
+
+			p := &CommitReportingPlugin{}


IMO it's easier to read when a struct is initalized in one instruction instead of being splitted into multiple p.field = "x". When I see init split into multiple lines, I would assume that we need some extra computation steps between adding values to the fields. Up to you.

dimkouv added 16 commits September 5, 2023 16:48

init and re-write commit observation

97a0e6a

fix commit report phase

8cc66b3

fix commit report encoding test

45919b1

remove non-plugin test from plugin test suite

2dc608b

cleanup commit shouldTransmit test

cfaa333

test commit shouldAccept phase, remove commit test harness

918a932

test/refactor exec buildReport

2c675f3

test exec report to eth tx meta

7c53799

fix commit report tests

6841023

write remaining tests for exec plugin + delete test harness

8c88c80

fix linter error

8f6e74c

TestCommitReportingPlugin_calculateMinMaxSequenceNumbers

42555e7

test get latest price updates

5336c63

rename commit tests

06401eb

rename and reorder exec plugin tests

3cbcf56

test remaining exec plugin methods

e3990d9

dimkouv changed the title ~~[DRAFT] unit testing improvements~~ offchain - unit testing improvements Sep 7, 2023

Merge branch 'ccip-develop' into ref/improve-test-suite

7d525b0

dimkouv marked this pull request as ready for review September 8, 2023 08:49

dimkouv requested a review from a team as a code owner September 8, 2023 08:49

dimkouv requested review from RensR, connorwstein and mateusz-sekara September 8, 2023 08:49

dimkouv added 3 commits September 8, 2023 12:08

update wait for healthy postgres script

61a11ce

Merge branch 'chore/fix-wait-for-pg-script' into ref/improve-test-suite

9d0db6b

fix failing test

10e2029

mateusz-sekara reviewed Sep 11, 2023

View reviewed changes

connorwstein reviewed Sep 12, 2023

View reviewed changes

dimkouv added 2 commits September 14, 2023 16:12

merge from ccip-develop

31195b8

resolve linter errors

003873b

dimkouv added 7 commits September 14, 2023 17:05

gitignore vendor dir

4f0f91f

improve test

c0279a1

assert no err

e22e298

rename ccipevents.Client to ccipdata.Reader

71f3437

Merge branch 'ccip-develop' into ref/improve-test-suite

fadf7d3

add fake implementations

e754fe5

Merge branch 'ref/improve-test-suite' of github.com:smartcontractkit/…

9cfd91c

…ccip into ref/improve-test-suite

mateusz-sekara approved these changes Sep 18, 2023

View reviewed changes

dimkouv added 2 commits September 18, 2023 13:02

merge from ccip-develop

cc1eec5

rename 'events' to 'reader'

008782c

dimkouv merged commit b69f404 into ccip-develop Sep 18, 2023
84 checks passed

dimkouv deleted the ref/improve-test-suite branch September 18, 2023 11:26

asoliman92 pushed a commit that referenced this pull request Jul 31, 2024

offchain - unit testing improvements (#117)

5fac764

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

offchain - unit testing improvements #117

offchain - unit testing improvements #117

dimkouv commented Sep 6, 2023 •

edited

Loading

mateusz-sekara Sep 11, 2023

mateusz-sekara Sep 11, 2023

dimkouv Sep 14, 2023

mateusz-sekara Sep 11, 2023

mateusz-sekara Sep 11, 2023

dimkouv Sep 14, 2023

mateusz-sekara Sep 11, 2023

dimkouv Sep 14, 2023

mateusz-sekara Sep 11, 2023

dimkouv Sep 14, 2023

mateusz-sekara Sep 11, 2023

dimkouv Sep 14, 2023

mateusz-sekara Sep 18, 2023

connorwstein Sep 12, 2023

dimkouv Sep 14, 2023 •

edited

Loading

connorwstein Sep 14, 2023 •

edited

Loading

dimkouv Sep 15, 2023

connorwstein Sep 12, 2023

dimkouv Sep 14, 2023

connorwstein Sep 12, 2023

connorwstein Sep 12, 2023 •

edited

Loading

dimkouv Sep 14, 2023

mateusz-sekara left a comment

mateusz-sekara Sep 18, 2023

offchain - unit testing improvements #117

offchain - unit testing improvements #117

Conversation

dimkouv commented Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimkouv Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

connorwstein Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorwstein Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateusz-sekara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimkouv commented Sep 6, 2023 •

edited

Loading

dimkouv Sep 14, 2023 •

edited

Loading

connorwstein Sep 14, 2023 •

edited

Loading

connorwstein Sep 12, 2023 •

edited

Loading