Add UTs to Processor `Execute()` #1890

RodrigoVillar · 2025-01-23T22:52:34Z

This PR adds the following:

UTs to test the Execute() method of the processor
Adds additional errors for UTs and expands functionality of existing mocks

aaronbuchwald · 2025-01-24T00:39:52Z

internal/logging/noop.go

+// Copyright (C) 2024, Ava Labs, Inc. All rights reserved.
+// See the file LICENSE for licensing terms.
+
+package logging


Can we use logging.NoLog{} from AvalancheGo?

aaronbuchwald · 2025-01-24T00:45:44Z

chain/processor_test.go

+			workers: &workers.MockWorkers{
+				OnNewJob: func(_ int) (workers.Job, error) {
+					return &workers.MockJob{
+						OnWaitError: errMockWorkerJob,
+					}, nil
+				},
+			},


Could we use the actual implementation of workers and a signature that should fail verification instead of mocking it?

aaronbuchwald · 2025-01-24T00:48:24Z

chain/processor_test.go

+				tt.validityWindow = &validitywindowtest.MockTimeValidityWindow[*chain.Transaction]{}
+			}
+
+			testChain, err := chain.NewChain(


Can we construct only the processor rather than the chain?

aaronbuchwald · 2025-01-24T14:52:19Z

chain/processor_test.go

+			block: &chain.ExecutionBlock{
+				StatelessBlock: &chain.StatelessBlock{
+					Hght:      1,
+					Tmstmp:    testRules.GetMinEmptyBlockGap(),
+					StateRoot: invalidStateRoot,
+				},
+			},


Can we switch all instances to using an existing constructor where possible?

This ensures that we're using the actual code to produce a fully populated block where possible and reduces the number of paths where we may create a block instance with some fields left unpopulated (this will leave the bytes, id, and auth counts fields unpopulated).

aaronbuchwald · 2025-01-24T14:54:21Z

chain/processor_test.go

+			for k, v := range tt.state {
+				r.NoError(db.Put([]byte(k), v))
+			}
+			r.NoError(db.CommitToDB(ctx))
+
+			if tt.includeRoot {
+				root, err := db.GetMerkleRoot(ctx)
+				r.NoError(err)
+				tt.block.StateRoot = root
+			}
+
+			_, err = testChain.Execute(ctx, db, tt.block, tt.isNormalOp)


Can we change to use a function to return the block based off of a merkledb.View constructed from the state?

This way each test case clearly specifies whether it is using the parent's merkle root or a random value as opposed to relying on overriding it via includeRoot here.

joshua-kim · 2025-01-27T16:51:25Z

chain/metrics.go

@@ -53,7 +53,7 @@ func (em *executorMetrics) RecordExecutable() {
 	em.executable.Inc()
 }

-func newMetrics(reg *prometheus.Registry) (*chainMetrics, error) {
+func NewMetrics(reg *prometheus.Registry) (*chainMetrics, error) {


Exported functions shouldn't return unexported types because then the calling code is unable to use the type it's being given

This diff is unrelated to the PR's objective of adding unit tests, unstage

As per offline convo, going to export NewMetrics() and ChainMetrics for now. A future PR which resolves the chain test cyclic dependencies should un-export these.

joshua-kim · 2025-01-27T16:52:23Z

chain/processor.go

@@ -383,7 +383,7 @@ func (p *Processor) createBlockContext(
 	}
 	parentHeight, err := database.ParseUInt64(parentHeightRaw)
 	if err != nil {
-		return blockContext{}, fmt.Errorf("failed to parse parent height from state: %w", err)
+		return blockContext{}, fmt.Errorf("%w %w", ErrFailedToParseParentHeight, err)


Recommend adding some delimiter like in the previous error message to make this easier to read

Suggested change

return blockContext{}, fmt.Errorf("%w %w", ErrFailedToParseParentHeight, err)

return blockContext{}, fmt.Errorf("%w: %w", ErrFailedToParseParentHeight, err)

joshua-kim · 2025-01-27T16:53:02Z

chain/processor_test.go

+// Copyright (C) 2024, Ava Labs, Inc. All rights reserved.
+// See the file LICENSE for licensing terms.
+
+package chain_test


Why don't unit tests for chain live inside of chain?

There's a cyclic dependency between the chain package and the genesis package

The cyclic dependency is more of a symptom of the underlying problem which is that the tests depend on auth since we use their signing implementations. This dependency issue is easily fixable if we just create a test auth implementation - there really isn't a reason why this package needs to exist. I'm fine with us doing this in a different PR though, since this package already exists in the current state of the repo

joshua-kim · 2025-01-27T17:11:10Z

chain/processor_test.go

+	testRules := genesis.NewDefaultRules()
+	testRuleFactory := genesis.ImmutableRuleFactory{Rules: testRules}
+	createValidBlock := func(root ids.ID) (*chain.StatelessBlock, error) {
+		return chain.NewStatelessBlock(
+			ids.Empty,
+			time.Now().UnixMilli(),
+			1,
+			nil,
+			root,
+		)
+	}


I would just eat the cost of duplicate code here and declare these per test - even if these are types that are immutable sharing state across tests is harder to maintain a mental model of + can lead to issues if it becomes mutable in the future.

joshua-kim · 2025-01-27T17:13:11Z

chain/processor_test.go

+			expectedErr: chain.ErrFailedToFetchParentFee,
+		},
+		{
+			name: "failed VerifyExpiryReplayProtection",


nit: name this lowercase to be consistent, e.g fails replay protection

joshua-kim · 2025-01-27T17:49:27Z

chain/processor_test.go

+			if tt.validityWindow == nil {
+				tt.validityWindow = &validitywindowtest.MockTimeValidityWindow[*chain.Transaction]{}
+			}
+
+			if tt.workers == nil {
+				tt.workers = workers.NewSerial()
+			}


Why do we override this from the test? IMO it's better to have a single source of truth and define this in the test table instead of overriding it w/ a default in the test body

Updated tests to declare the workers/validity window that they use

joshua-kim · 2025-01-27T17:51:41Z

chain/processor_test.go

+		},
+		{
+			name: "block timestamp too late",
+			createBlock: func(root ids.ID) (*chain.StatelessBlock, error) {


IMO returning this error is awkward, can we use require.NoError(t, <type>) to guarantee no error to the caller?

joshua-kim · 2025-01-27T17:53:40Z

chain/processor_test.go

+		validityWindow chain.ValidityWindow
+		isNormalOp     bool
+		createBlock    createBlock
+		state          map[string][]byte


Since the function Execute that we're testing operates on a merkledb.View, should we use that instead of a map?

joshua-kim · 2025-01-27T17:54:27Z

chain/processor_test.go

+		isNormalOp     bool
+		createBlock    createBlock
+		state          map[string][]byte
+		workers        workers.Workers


nit: move this to be adjacent to validityWindow so that dependencies and parameters are grouped to be adjacent with each other

joshua-kim · 2025-01-27T17:56:39Z

chain/processor_test.go

+			state: map[string][]byte{
+				heightKey:    binary.BigEndian.AppendUint64(nil, 0),
+				timestampKey: binary.BigEndian.AppendUint64(nil, 0),
+				feeKey:       {},


q: Why is this value the empty value? Is this for convenience because the zero value {} is equivalent to a 0 varint?

By setting the raw fee value to empty, this is equivalent to a fresh fee manager being loaded in (and which should be fine since we're not testing the FM in this PR)

hypersdk/internal/fees/manager.go

Lines 33 to 35 in a9801b8

if len(raw) == 0 {

raw = make([]byte, consts.Int64Len+fees.FeeDimensions*dimensionStateLen)

}

chain/processor_test.go

joshua-kim · 2025-01-28T22:10:16Z

chain/processor_test.go

+	createValidBlock := func(root ids.ID) *chain.StatelessBlock {
+		block, err := chain.NewStatelessBlock(
+			ids.Empty,
+			time.Now().UnixMilli(),


nit: avoid non-deterministic behavior in unit tests where possible - this test should pass if we just use time.Time{}

Done; only exception is the "block timestamp too late" test which uses time.Now() to error against the following:

hypersdk/chain/processor.go

Lines 130 to 133 in a9801b8

// Perform basic correctness checks before doing any expensive work

if b.Tmstmp > time.Now().Add(FutureBound).UnixMilli() {

return nil, ErrTimestampTooLate

}

This test is still inherently flaky, it could be refactored to accept caller-provided timestamps but it's outside of the scope of the PR

chain/processor_test.go

aaronbuchwald · 2025-01-30T15:41:55Z

chain/processor_test.go

+		validityWindow chain.ValidityWindow
+		workers        workers.Workers
+		isNormalOp     bool
+		db             merkledb.MerkleDB


Could we use merkledb.View instead of merkledb.MerkleDB ?

aaronbuchwald · 2025-01-30T15:42:58Z

chain/processor_test.go

+	tests := []struct {
+		name           string
+		validityWindow chain.ValidityWindow
+		workers        workers.Workers


Why do we need workers.Workers here?

Removed workers from tests

aaronbuchwald · 2025-01-30T15:45:08Z

chain/processor_test.go

+			name:           "verify signatures fails",
+			validityWindow: &validitywindowtest.MockTimeValidityWindow[*chain.Transaction]{},
+			workers: func() workers.Workers {
+				w := workers.NewParallel(0, 0)
+				w.Stop()
+				return w
+			}(),


Can we test this by using an invalid auth rather than mocking workers? I don't think it's necessary to test with an invalid instance of workers.Workers so we should remove this as a parameter from the test cases.

As per offline convo, I believe we can get rid of this test. Stop() is called when the VM is shutting down and since that case is outside the scope of Execute(), we shouldn't need to test for it.

aaronbuchwald · 2025-01-30T15:46:02Z

chain/processor_test.go

+			db: func() merkledb.MerkleDB {
+				db, err := merkledb.New(
+					context.Background(),
+					memdb.New(),
+					merkledb.Config{
+						BranchFactor: merkledb.BranchFactor16,
+						Tracer:       trace.Noop,
+					},
+				)
+				require.NoError(t, err)
+				return db
+			}(),


can we replace constructing the merkledb each time with a function that takes in a set of state keys and then returns a merkledb.View with all of those added so that we don't need to repeat the same code in every test case?

aaronbuchwald · 2025-01-30T15:52:57Z

chain/processor_test.go

+			newBlockF: func(root ids.ID) *chain.StatelessBlock {
+				block, err := chain.NewStatelessBlock(
+					ids.Empty,
+					genesis.NewDefaultRules().GetMinEmptyBlockGap(),


If we are using genesis.NewDefaultRules() in the body of the for loop, then it's more cognitive load imo to use genesis.NewDefaultRules(), which depends on the fact we're using the default below.

Could we define the rules at the top of the test rather than doing this in each test case where it's needed?

aaronbuchwald · 2025-01-30T16:44:41Z

chain/processor_test.go

+				chain.NewExecutionBlock(tt.newBlockF(root)),
+				tt.isNormalOp,
+			)
+			r.ErrorIs(err, tt.expectedErr)


(separate PR) - could we confirm the post state is what we expect as well ie. we should make sure that the metadata is updated correctly and that tx/action execution (include successful and failed w/ rollbacks) has the expected effect

init

fe432e2

RodrigoVillar requested review from aaronbuchwald and joshua-kim as code owners January 23, 2025 22:52

nit

81b93f8

aaronbuchwald requested changes Jan 24, 2025

View reviewed changes

RodrigoVillar added 7 commits January 24, 2025 12:43

Merge branch 'main' into add-processor-uts

12f310d

use avalanchego logger

3e9f766

create processor instead of chain

467d7b8

use constructors and lambda funcs

5cdc674

use constructor for tx

d3920c0

nit

dd9c5a9

consistent block timestamps

d2da050

RodrigoVillar requested a review from aaronbuchwald January 27, 2025 15:14

RodrigoVillar self-assigned this Jan 27, 2025

joshua-kim reviewed Jan 27, 2025

View reviewed changes

RodrigoVillar added 12 commits January 28, 2025 13:21

Merge branch 'main' into add-processor-uts

8791a01

add delimiter

37dc75c

consistent naming

3fa517d

group dependencies/parameters

3598dc7

each test defines worker

33e0fe4

each test defines validitywindow

2bbc489

createBlock returns only block

390c816

consistent usage of require

7aa3fd1

each test declares rules

9cd1db1

remove unnecessary commit to db

3395a71

replace map with merkledb

c5750f8

export chain metrics

167755c

RodrigoVillar requested a review from joshua-kim January 28, 2025 21:40

joshua-kim reviewed Jan 28, 2025

View reviewed changes

nits

d56316e

RodrigoVillar added 8 commits January 28, 2025 17:24

nits

8b4f12e

rename block constructor

a51526d

use single time

6acef69

nit

d9e0a43

use constant time when possible

c0934f6

nit

de7142d

nit

7f3e6ea

remove lambda funcs

6afe172

RodrigoVillar requested a review from joshua-kim January 29, 2025 14:53

joshua-kim previously approved these changes Jan 29, 2025

View reviewed changes

joshua-kim enabled auto-merge (squash) January 29, 2025 20:23

Merge branch 'main' into add-processor-uts

d23cd97

aaronbuchwald requested changes Jan 30, 2025

View reviewed changes

RodrigoVillar added 5 commits January 30, 2025 16:02

use single genesis rules

0ef1f29

replace db with view

fc22e50

add view helper function

4b12eee

nit

c4aeff6

nit

f5036f5

RodrigoVillar dismissed joshua-kim’s stale review via f5036f5 January 30, 2025 21:02

RodrigoVillar added 2 commits January 30, 2025 16:19

remove unnecessary workers

0040c01

Merge branch 'main' into add-processor-uts

9b6b114

	return blockContext{}, fmt.Errorf("%w %w", ErrFailedToParseParentHeight, err)
	return blockContext{}, fmt.Errorf("%w: %w", ErrFailedToParseParentHeight, err)

	if len(raw) == 0 {
	raw = make([]byte, consts.Int64Len+fees.FeeDimensions*dimensionStateLen)
	}

	// Perform basic correctness checks before doing any expensive work
	if b.Tmstmp > time.Now().Add(FutureBound).UnixMilli() {
	return nil, ErrTimestampTooLate
	}

Add UTs to Processor Execute() #1890

Are you sure you want to change the base?

Add UTs to Processor Execute() #1890

Conversation

RodrigoVillar commented Jan 23, 2025 • 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

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

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

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

Add UTs to Processor `Execute()` #1890

Add UTs to Processor `Execute()` #1890

RodrigoVillar commented Jan 23, 2025 •

edited

Loading