Unit tests for `node` package + small refactoring & fixes #5132

iulianpascalau · 2023-03-28T15:25:51Z

Reasoning behind the pull request

added unit tests for increasing code coverage

Proposed changes

added unit tests for the node runner

Testing procedure

standard system test. Stopping the node & shuffling out should work, also, the stats directory should now contain the genesisSmartContracts.json and the gasSchedules directory and all the gas schedules files.

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

was the PR targeted to the correct branch?
if this is a larger feature that probably needs more than one PR, is there a feat branch created?
if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

codecov-commenter · 2023-03-28T15:39:35Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +1.13 🎉

Comparison is base (6a1b1c8) 75.74% compared to head (ede0314) 76.87%.

❗ Current head ede0314 differs from pull request most recent head 8a5f2d4. Consider uploading reports for the commit 8a5f2d4 to get more accurate results

Additional details and impacted files

@@              Coverage Diff              @@
##           rc/v1.5.0    #5132      +/-   ##
=============================================
+ Coverage      75.74%   76.87%   +1.13%     
=============================================
  Files            650      650              
  Lines          85464    85470       +6     
=============================================
+ Hits           64735    65708     +973     
+ Misses         15639    14555    -1084     
- Partials        5090     5207     +117

Impacted Files	Coverage Δ
node/metrics/metrics.go	`100.00% <100.00%> (+35.44%)`	⬆️
node/nodeRunner.go	`74.00% <100.00%> (+74.00%)`	⬆️

... and 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

… into add-unit-tests-for-node-package

iulianpascalau · 2023-03-30T06:44:43Z

node/mock/applicationRunningTrigger.go

+	}
+}
+
+func (trigger *applicationRunningTrigger) Write(p []byte) (n int, err error) {


missing comment

added, thanks

iulianpascalau · 2023-03-30T06:47:51Z

node/nodeRunner_test.go

+	// src
+	//   +- file2
+	//   +- dir1
+	//	       +- file3


indentation error

raduchis · 2023-03-30T08:19:13Z

node/nodeRunner_test.go

+	err := logger.AddLogObserver(trigger, &logger.PlainFormatter{})
+	require.Nil(t, err)
+
+	// start a go routine that will send the SIGINT message after 1 minute


after 1 second (after the application started)?

yes, indeed, rotten comment.

sstanculeanu · 2023-03-30T10:47:49Z

node/metrics/metrics_test.go

+		for k, v := range expectedValues {
+			assert.Equal(t, v, keys[k], fmt.Sprintf("for key %s", k))
+		}
+	})


addition 3: extra tests here

t.Run("should work - metachain", func(t *testing.T) { t.Parallel() keys := make(map[string]interface{}) localStatusHandler := &statusHandler.AppStatusHandlerStub{ SetUInt64ValueHandler: func(key string, value uint64) { keys[key] = value }, SetStringValueHandler: func(key string, value string) { keys[key] = value }, } localShardCoordinator := &testscommon.ShardsCoordinatorMock{ NoShards: 3, SelfIDCalled: func() uint32 { return common.MetachainShardId }, } err := InitMetrics(localStatusHandler, pubkeyString, nodeType, localShardCoordinator, nodesSetup, version, economicsConfigs, roundsPerEpoch, minTransactionVersion) assert.Nil(t, err) expectedValues := map[string]interface{}{ common.MetricPublicKeyBlockSign: pubkeyString, common.MetricShardId: uint64(localShardCoordinator.SelfId()), common.MetricNumShardsWithoutMetachain: uint64(localShardCoordinator.NoShards), common.MetricNodeType: string(nodeType), common.MetricRoundTime: uint64(6), common.MetricAppVersion: version, common.MetricRoundsPerEpoch: uint64(roundsPerEpoch), common.MetricCrossCheckBlockHeight: "0", common.MetricCrossCheckBlockHeight + "_0": uint64(0), common.MetricCrossCheckBlockHeight + "_1": uint64(0), common.MetricCrossCheckBlockHeight + "_2": uint64(0), common.MetricCrossCheckBlockHeightMeta: uint64(0), common.MetricIsSyncing: uint64(1), common.MetricLeaderPercentage: fmt.Sprintf("%f", 2.0), common.MetricDenomination: uint64(4), common.MetricShardConsensusGroupSize: uint64(63), common.MetricMetaConsensusGroupSize: uint64(400), common.MetricNumNodesPerShard: uint64(402), common.MetricNumMetachainNodes: uint64(401), common.MetricStartTime: uint64(111111), common.MetricRoundDuration: uint64(6000), common.MetricMinTransactionVersion: uint64(1), common.MetricNumValidators: uint64(0), common.MetricConsensusGroupSize: uint64(400), } assert.Equal(t, len(expectedValues), len(keys)) for k, v := range expectedValues { assert.Equal(t, v, keys[k], fmt.Sprintf("for key %s", k)) } }) t.Run("should work - invalid shard id", func(t *testing.T) { t.Parallel() keys := make(map[string]interface{}) localStatusHandler := &statusHandler.AppStatusHandlerStub{ SetUInt64ValueHandler: func(key string, value uint64) { keys[key] = value }, SetStringValueHandler: func(key string, value string) { keys[key] = value }, } localShardCoordinator := &testscommon.ShardsCoordinatorMock{ NoShards: 3, SelfIDCalled: func() uint32 { return 10 }, } err := InitMetrics(localStatusHandler, pubkeyString, nodeType, localShardCoordinator, nodesSetup, version, economicsConfigs, roundsPerEpoch, minTransactionVersion) assert.Nil(t, err) assert.Equal(t, uint64(0), keys[common.MetricConsensusGroupSize]) })

added, thanks

sstanculeanu · 2023-03-30T10:49:10Z

node/metrics/metrics_test.go

+				{
+					LeaderPercentage: 2,
+				},


addition 2: extra entry here:

Suggested change

{

LeaderPercentage: 2,

},

{

LeaderPercentage: 2,

},

{

LeaderPercentage: 2,

},

sstanculeanu · 2023-03-30T10:52:05Z

node/metrics/metrics_test.go

@@ -329,3 +332,185 @@ func TestInitRatingsMetrics(t *testing.T) {
 		assert.Equal(t, v, keys[k])
 	}
 }
+
+func TestInitMetrics(t *testing.T) {


100% coverage can be reached on this component using the next 3 additions:
addition 1:

on L140, as part of TestInitConfigMetrics, add MaxNodesChangeEnableEpoch

MaxNodesChangeEnableEpoch: []config.MaxNodesChangeConfig{ { EpochEnable: 0, MaxNumNodes: 1, NodesToShufflePerShard: 2, }, },

then on L190 we'll need new expected metrics:

"erd_max_nodes_change_enable_epoch0_epoch_enable": uint32(0), "erd_max_nodes_change_enable_epoch0_max_num_nodes": uint32(1), "erd_max_nodes_change_enable_epoch0_nodes_to_shuffle_per_shard": uint32(2),

added, thanks.

sstanculeanu · 2023-03-30T11:23:12Z

node/nodeRunner_test.go

+		)
+
+		// these exceptions appear because the delayedComponent prevented the call of the first 2 components
+		// as the closable components are called in revered order


Suggested change

// as the closable components are called in revered order

// as the closable components are called in reversed order

sstanculeanu · 2023-03-30T11:23:52Z

node/nodeRunner_test.go

+		)
+
+		// these exceptions appear because the delayedComponent prevented the call of the first 2 components
+		// as the closable components are called in revered order


Suggested change

// as the closable components are called in revered order

// as the closable components are called in reversed order

sstanculeanu · 2023-03-30T11:24:32Z

node/nodeRunner_test.go

+	assert.Equal(tb, numKeys, len(closedCalled))
+}
+
+func contains(needle string, haystack []string) bool {


gabi-vuls

System test passed.
@@ Log scanner @@

add-unit-tests-for-node-package

================================================================================

Known Warnings 14
New Warnings 2
Known Errors 0
New Errors 1
Panics 0
================================================================================
block hash does not match 10938
wrong nonce in block 3558
miniblocks does not match 0
num miniblocks does not match 0
miniblock hash does not match 0
block bodies does not match 0
receipts hash missmatch 0
================================================================================
No jailed nodes on the thestnet
================================================================================

iulianpascalau

Refactored some tests due to this comment: #5141 (comment)

sstanculeanu · 2023-03-31T15:43:47Z

node/external/timemachine/fee/feeComputer_test.go

@@ -216,3 +216,18 @@ func TestFeeComputer_InHighConcurrency(t *testing.T) {

 	wg.Wait()
 }
+
+func TestFullHistoryPruningStorer_IsInterfaceNil(t *testing.T) {


Suggested change

func TestFullHistoryPruningStorer_IsInterfaceNil(t *testing.T) {

func TestFeeComputer_IsInterfaceNil(t *testing.T) {

too much copy-paste :D
Thanks.

raduchis · 2023-03-31T16:47:49Z

node/external/logs/logsFacade_test.go

+	t.Parallel()
+
+	var lf *logsFacade
+	require.True(t, check.IfNil(lf))


Suggested change

require.True(t, check.IfNil(lf))

require.True(t, lf.IsInterfaceNil())

done on the whole package
Kept check.IfNil call whenever the function under test returned an interface (compatible with NilInterfaceChecker).

raduchis · 2023-03-31T16:51:18Z

node/external/logs/logsFacade_test.go

+		PubKeyConverter: testscommon.NewPubkeyConverterMock(32),
+	}
+	lf, _ = NewLogsFacade(arguments)
+	require.False(t, check.IfNil(lf))


Suggested change

require.False(t, check.IfNil(lf))

require.False(t, lf.IsInterfaceNil())

- added unit tests for the node runner

bbff28a

iulianpascalau added 3 commits March 28, 2023 19:52

- added unit tests for the node.go

786af3b

- added tests for metrics.go

bae8fe1

- added more tests, refactored existing code

2525932

iulianpascalau changed the base branch from master to rc/v1.5.0 March 29, 2023 13:00

Merge branch 'rc/v1.5.0' into add-unit-tests-for-node-package

d9d960e

iulianpascalau changed the title ~~[WIP]Unit tests for node package~~ Unit tests for node package + small refactoring Mar 29, 2023

iulianpascalau changed the title ~~Unit tests for node package + small refactoring~~ Unit tests for node package + small refactoring & fixes Mar 29, 2023

iulianpascalau self-assigned this Mar 29, 2023

iulianpascalau added impact:low interested:protocol size:small type:refactoring complexity:medium area:testing labels Mar 29, 2023

iulianpascalau marked this pull request as ready for review March 29, 2023 13:01

iulianpascalau added 3 commits March 29, 2023 16:04

- added missing stub

abb70bc

Merge remote-tracking branch 'origin/add-unit-tests-for-node-package'…

0c0fa82

… into add-unit-tests-for-node-package

- fixed test name

4ba6419

iulianpascalau commented Mar 30, 2023

View reviewed changes

- fix after review - part 1

12fe487

raduchis self-requested a review March 30, 2023 07:09

raduchis previously approved these changes Mar 30, 2023

View reviewed changes

- fix after review - part 2

12d8046

iulianpascalau dismissed raduchis’s stale review via 12d8046 March 30, 2023 08:42

raduchis previously approved these changes Mar 30, 2023

View reviewed changes

sstanculeanu self-requested a review March 30, 2023 10:25

sstanculeanu reviewed Mar 30, 2023

View reviewed changes

- fixes after review - part 3

bf923c7

iulianpascalau dismissed raduchis’s stale review via bf923c7 March 30, 2023 12:05

sstanculeanu previously approved these changes Mar 30, 2023

View reviewed changes

raduchis previously approved these changes Mar 30, 2023

View reviewed changes

gabi-vuls previously approved these changes Mar 31, 2023

View reviewed changes

- refactored the IsInterfaceNil tests

56bf141

iulianpascalau dismissed stale reviews from gabi-vuls, raduchis, and sstanculeanu via 56bf141 March 31, 2023 15:37

iulianpascalau commented Mar 31, 2023

View reviewed changes

sstanculeanu reviewed Mar 31, 2023

View reviewed changes

- fixed test name

d4ab76f

sstanculeanu previously approved these changes Mar 31, 2023

View reviewed changes

raduchis reviewed Mar 31, 2023

View reviewed changes

refactor part 2 for the IsInterfaceNil tests

b9953f4

iulianpascalau dismissed sstanculeanu’s stale review via b9953f4 March 31, 2023 17:44

iulianpascalau changed the title ~~Unit tests for node package + small refactoring & fixes~~ Unit tests for node package + small refactoring & fixes Mar 31, 2023

sstanculeanu approved these changes Mar 31, 2023

View reviewed changes

raduchis approved these changes Apr 1, 2023

View reviewed changes

gabi-vuls approved these changes Apr 3, 2023

View reviewed changes

Merge branch 'rc/v1.5.0' into add-unit-tests-for-node-package

8a5f2d4

iulianpascalau merged commit 31f14ca into rc/v1.5.0 Apr 3, 2023

iulianpascalau deleted the add-unit-tests-for-node-package branch April 3, 2023 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit tests for `node` package + small refactoring & fixes #5132

Unit tests for `node` package + small refactoring & fixes #5132

iulianpascalau commented Mar 28, 2023 •

edited

Loading

codecov-commenter commented Mar 28, 2023 •

edited by codecov bot

Loading

iulianpascalau Mar 30, 2023

iulianpascalau Mar 30, 2023

iulianpascalau Mar 30, 2023

iulianpascalau Mar 30, 2023

raduchis Mar 30, 2023

iulianpascalau Mar 30, 2023

sstanculeanu Mar 30, 2023

iulianpascalau Mar 30, 2023

sstanculeanu Mar 30, 2023

iulianpascalau Mar 30, 2023

sstanculeanu Mar 30, 2023

iulianpascalau Mar 30, 2023

sstanculeanu Mar 30, 2023

iulianpascalau Mar 30, 2023

sstanculeanu Mar 30, 2023

iulianpascalau Mar 30, 2023

sstanculeanu Mar 30, 2023

gabi-vuls left a comment

iulianpascalau left a comment

sstanculeanu Mar 31, 2023

iulianpascalau Mar 31, 2023

raduchis Mar 31, 2023

iulianpascalau Mar 31, 2023 •

edited

Loading

raduchis Apr 1, 2023

raduchis Mar 31, 2023

	// as the closable components are called in revered order
	// as the closable components are called in reversed order

	func TestFullHistoryPruningStorer_IsInterfaceNil(t *testing.T) {
	func TestFeeComputer_IsInterfaceNil(t *testing.T) {

	require.True(t, check.IfNil(lf))
	require.True(t, lf.IsInterfaceNil())

	require.False(t, check.IfNil(lf))
	require.False(t, lf.IsInterfaceNil())

Unit tests for node package + small refactoring & fixes #5132

Unit tests for node package + small refactoring & fixes #5132

Conversation

iulianpascalau commented Mar 28, 2023 • edited Loading

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

codecov-commenter commented Mar 28, 2023 • edited by codecov bot Loading

Codecov Report

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

gabi-vuls left a comment

Choose a reason for hiding this comment

add-unit-tests-for-node-package

iulianpascalau left a 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

iulianpascalau Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Unit tests for `node` package + small refactoring & fixes #5132

Unit tests for `node` package + small refactoring & fixes #5132

iulianpascalau commented Mar 28, 2023 •

edited

Loading

codecov-commenter commented Mar 28, 2023 •

edited by codecov bot

Loading

iulianpascalau Mar 31, 2023 •

edited

Loading