testing (dot) create dot unit tests #1980

edwardmack · 2021-11-03T20:52:41Z

Changes

Create unit tests dot package: build_spec_test.go, config_test.go, import_test.go, node_test.go, service_test.go and utils_test.go

Tests

go test ./dot/ -v

Issues

closes Create dot unit tests #1952

Primary Reviewer

@timwu20, @qdm12

qdm12 · 2021-11-04T13:17:50Z

I see a lot of integration tests deltas, is this expected? Did you maybe branch off your other integration tests PR?

TestBuildSpec_ToJSON and TestBuildSpec_ToJSONRaw do not have test for errors with json.Marshal, I don't know how to create condition that will simulate an error with this.

JSON Marshal will fail if you try to marshal e.g. a function/channel. Maybe add some dummy function field to your field struct to trigger an error?

TestWriteGenesisSpecFile doesn't test for errors from os.MkdirAll, I don't know how to create condition to simulate error.

Have a file at the same path 😉 Example

/tmp # touch test
/tmp # mkdir -p test
mkdir: can't create directory 'test': File exists

since these functions build the struct I don't how to compare pointer addresses

Why would we care about the pointer address?

Cleanup after errors

Shouldn't it cleanup even when there is no error? 🤔

Other general comments

For testing errors, I would suggest to use (since we already use testify around in the repo):

if testCase.err != nil {
	assert.EqualError(t, err, testCase.err.Error())
} else {
	assert.NoError(t, err)
}

You can use assert.Equal(t, expected, actual) instead of reflect.DeepEqual + t.Errorf(...) since it does the same thing, it would accelerate test writing/shorten your test code 😉

edwardmack · 2021-11-05T20:42:15Z

I see a lot of integration tests deltas, is this expected? Did you maybe branch off your other integration tests PR?

I did branch off my other integration tests PR, is this a bad idea? Whats the best way to work on stuff that depends on other stuff you've done. I've gotten into git branch trouble before, so I'm open to suggestions of best way to organize this.

JSON Marshal will fail if you try to marshal e.g. a function/channel. Maybe add some dummy function field to your field struct to trigger an error?

I'm no longer sure I need to test for this since the struct passed to this function insures it will not receive a function/channel.

TestWriteGenesisSpecFile doesn't test for errors from os.MkdirAll, I don't know how to create condition to simulate error.

Have a file at the same path wink Example
/tmp # touch test
/tmp # mkdir -p test
mkdir: can't create directory 'test': File exists

I wasn't able to create condition where os.MkdirAll failed.

since these functions build the struct I don't how to compare pointer addresses

Why would we care about the pointer address?

Good point, I've updated tests to compare returned values.

Cleanup after errors

Shouldn't it cleanup even when there is no error? thinking

Yes, good point, I've updated this.

Other general comments
For testing errors, I would suggest to use (since we already use testify around in the repo):
if testCase.err != nil {
	assert.EqualError(t, err, testCase.err.Error())
} else {
	assert.NoError(t, err)
}
You can use assert.Equal(t, expected, actual) instead of reflect.DeepEqual + t.Errorf(...) since it does the same thing, it would accelerate test writing/shorten your test code wink

Updated.

codecov · 2021-11-05T21:02:11Z

Codecov Report

Merging #1980 (f3936af) into development (1e74231) will increase coverage by 0.59%.
The diff coverage is 61.66%.

@@               Coverage Diff               @@
##           development    #1980      +/-   ##
===============================================
+ Coverage        61.34%   61.93%   +0.59%     
===============================================
  Files              203      203              
  Lines            27348    27400      +52     
===============================================
+ Hits             16776    16970     +194     
+ Misses            8699     8558     -141     
+ Partials          1873     1872       -1

Flag	Coverage Δ
unit-tests	`61.93% <61.66%> (+0.59%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/blocktree/blocktree.go	`53.81% <0.00%> (-1.10%)`	⬇️
dot/core/service.go	`54.84% <57.14%> (-0.60%)`	⬇️
lib/babe/verify.go	`60.05% <57.14%> (+0.23%)`	⬆️
dot/node.go	`50.99% <60.43%> (-5.16%)`	⬇️
dot/services.go	`72.29% <100.00%> (+4.14%)`	⬆️
lib/blocktree/node.go	`63.50% <0.00%> (-7.30%)`	⬇️
dot/network/connmgr.go	`89.04% <0.00%> (-1.37%)`	⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e74231...f3936af. Read the comment docs.

qdm12 · 2021-11-06T19:22:21Z

I did branch off my other integration tests PR, is this a bad idea? Whats the best way to work on stuff that depends on other stuff you've done. I've gotten into git branch trouble before, so I'm open to suggestions of best way to organize this.

I'd suggest you point this PR to that other branch you branched out from, merge the second one (this one) first and then the other branch (the first one) in development.

And yes don't worry too much about impossible to reproduce errors 😄 I'll mess around with os.MkdirAll though I'm curious now!

timwu20 · 2021-11-08T15:55:41Z

Can you change the base branch of this PR to the other branch you moved the integration tests too? It'll make the diff easier to read.

timwu20 · 2021-11-08T16:17:10Z

dot/build_spec_test.go

+	}{
+		{
+			name:   "normal conditions",
+			fields: fields{genesis: &genesis.Genesis{Name: "test"}},


can you add more tests to test the other attributes of genesis.Genesis

timwu20 · 2021-11-08T16:22:46Z

I wouldn't worry about trying to test coverage up to 100%. It's not really necessary to test json.Marshal and file read errors if you're already above 70% coverage.

qdm12 · 2021-11-10T12:07:43Z

dot/node.go

@@ -180,7 +180,7 @@ func NodeInitialized(basepath string) bool {
 }

 // LoadGlobalNodeName returns the stored global node name from database
-func LoadGlobalNodeName(basepath string) (nodename string, err error) {
+func LoadGlobalNodeName(basepath string) (string, error) {


Please let's use named returned values, it's really useful for readers/documentation.

qdm12 · 2021-11-11T14:55:00Z

dot/services_test.go

-			if !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("createCoreService() got = %v, want %v", got, tt.want)
+			if tt.want != nil {
+				assert.NotNil(t, got)


Maybe assert it's nil if we expect it to be nil as well 🤔 ?

Good idea, added.

qdm12 · 2021-11-11T14:55:41Z

dot/services_test.go

 			}
 		})
 	}
 }

 func Test_createDigestHandler(t *testing.T) {
+	cfg := NewTestConfig(t)
+	require.NotNil(t, cfg)


nit I don't think this NotNil is really required since (afaik) we are asserting something from the test (NewTestConfig). Same for below checks as well. Although if these helper functions do use some production code, maybe do the assertion inside them since you are already passing t to them.

Ok, removed.

qdm12 · 2021-11-11T14:57:21Z

dot/services_test.go

+				st:   stateSrvc,
+				code: []byte(`fake code`),
+			},
+			err: errors.New("failed to create runtime executor: Failed to instantiate the module:\n    compile error: Validation error \"Bad magic number\""),


Is it us or the runtime returning an error with \n?

qdm12 · 2021-11-11T14:59:35Z

lib/babe/verify.go

+	switch blockState.(type) {
+	case *state.BlockState:
+		if blockState == (*state.BlockState)(nil) {
+			return nil, errNilBlockState
+		}
+	default:
+		if reflect.ValueOf(blockState).IsNil() {
+			return nil, errNilBlockState
+		}
 	}


supernit to reduce code duplication if we add more cases 😉

Suggested change

switch blockState.(type) {

case *state.BlockState:

if blockState == (*state.BlockState)(nil) {

return nil, errNilBlockState

}

default:

if reflect.ValueOf(blockState).IsNil() {

return nil, errNilBlockState

}

}

var isNil bool

switch blockState.(type) {

case *state.BlockState:

isNil = blockState == (*state.BlockState)(nil)

default:

isNil = reflect.ValueOf(blockState).IsNil()

}

if isNil {

return nil, errNilBlockState

}

Bumps [actions/setup-node](https://github.com/actions/setup-node) from 2.4.1 to 2.5.0. - [Release notes](https://github.com/actions/setup-node/releases) - [Commits](actions/setup-node@v2.4.1...v2.5.0) --- updated-dependencies: - dependency-name: actions/setup-node dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

noot · 2021-12-15T18:08:30Z

dot/build_spec_test.go

+	//<<<<<<< HEAD
+	//=======
+	//
+	//	bs.genesis.ChainType = expectedChainType
+	//	bs.genesis.Properties = expectedProperties
+	//
+	//	require.NoError(t, err)
+	//
+	//	// confirm human-readable fields


remove commented out block

noot · 2021-12-15T18:08:40Z

dot/build_spec_test.go

+	//		gen := new(genesis.Genesis)
+	//		err = json.Unmarshal(genesisBytes, gen)
+	//		require.NoError(t, err)
+	//>>>>>>> development


noot · 2021-12-15T18:09:18Z

dot/node.go

-	return node, nil
+}
+
+//InitKeystore to initialize node keystore


Suggested change

//InitKeystore to initialize node keystore

// InitKeystore initialised the node's keystore

noot · 2021-12-15T18:10:44Z

dot/node.go

@@ -41,9 +48,87 @@ type Node struct {
 	started  chan struct{}
 }

+//go:generate mockgen -source=node.go -destination=mock_node_test.go -package=$GOPACKAGE
+
+type newNodeIface interface {


nice! I like this interface a lot, maybe name it nodeBuilder or nodeCreator or something like that?

noot · 2021-12-15T18:12:53Z

lib/babe/verify.go

+	var isNilBlock bool
+	switch blockState.(type) {
+	case *state.BlockState:
+		isNilBlock = blockState == (*state.BlockState)(nil)
+	default:
+		isNilBlock = reflect.ValueOf(blockState).IsNil()
+	}
+	if isNilBlock {
 		return nil, errNilBlockState
 	}

-	if epochState == nil {
+	var isNilEpoch bool
+	switch epochState.(type) {
+	case *state.EpochState:
+		isNilEpoch = epochState == (*state.EpochState)(nil)
+	default:
+		isNilEpoch = reflect.ValueOf(epochState).IsNil()
+	}
+	if isNilEpoch {


do we ned this isNilBlock = blockState == (*state.BlockState)(nil) or does reflect.ValueOf(blockState).IsNil() evaluate to the same thing? I'd rather not have the (*state.BlockState) type assertion if possible

noot · 2021-12-15T18:13:17Z

lib/blocktree/blocktree.go

@@ -403,7 +403,7 @@ func (bt *BlockTree) StoreRuntime(hash common.Hash, in runtime.Instance) {
 // GetBlockRuntime returns block runtime for corresponding block hash.
 func (bt *BlockTree) GetBlockRuntime(hash common.Hash) (runtime.Instance, error) {
 	ins, ok := bt.runtime.Load(hash)
-	if !ok {
+	if !ok || ins == nil {


is ins == nil a case that happens? that seems concerning to me lol

noot

very nice work! just a few comments, but the tests look good to me :D

edwardmack · 2022-01-11T15:52:11Z

closed, replaced by PR #2112

edwardmack added 10 commits November 2, 2021 16:33

move build_spec_tests

6b61943

added file

d12b7c2

move config_tests

2346cd9

move import_tests

9ac48f4

move node_tests

f10a4db

move services_tests

8cafe33

fix name

9042ce9

move utils_tests

1254342

Merge branch 'development' into ed/dot_tests_to_integration

035398c

add build_spec unit tests

974b3b2

edwardmack added 2 commits November 5, 2021 14:18

Merge branch 'development' into ed/create_dot_unit_tests

e206c0c

refactor tests

73d870a

lint

84321d7

generate tests stubs

28ce808

timwu20 reviewed Nov 8, 2021

View reviewed changes

edwardmack added 4 commits November 8, 2021 16:07

implement core config unit tests

6b3cd93

add t.Parallel to tests

357dd19

implement tests for import.go

b43f571

add tests for node.go

49c3add

qdm12 reviewed Nov 10, 2021

View reviewed changes

implement service tests

cecc641

qdm12 reviewed Nov 11, 2021

View reviewed changes

edwardmack added 2 commits November 11, 2021 10:20

Merge branch 'development' into ed/create_dot_unit_tests

d5cee6c

implement services tests

3cfdf93

dependabot bot and others added 5 commits December 6, 2021 12:03

lint

2a18498

feat(wasmer/crypto): move sig verifier to crypto pkg (#2057)

2842ba7

remove testing NewNodeB

219ca79

Merge branch 'development' into ed/create_dot_unit_tests

e651076

timwu20 force-pushed the development branch from 2842ba7 to dc8bbef Compare December 6, 2021 20:59

edwardmack added 5 commits December 6, 2021 16:41

change init keystore

d972258

remove newNodeC

cdbe54c

remove gomocks.Any from tests

1d05a8d

remove naked return

eeb1115

rename e to closeErr

c84ca41

edwardmack marked this pull request as ready for review December 7, 2021 22:56

edwardmack added 8 commits December 8, 2021 11:00

Merge branch 'development' into ed/create_dot_unit_tests

ecab259

WIP save

2830f7b

fix Test_createGRANDPAService test to use mocks

1ef3897

fix failing tests

29d8975

add to comment

2880f52

Merge branch 'development' into ed/create_dot_unit_tests

c71c873

test commit

5665356

test commit

f3936af

noot reviewed Dec 15, 2021

View reviewed changes

edwardmack closed this Jan 11, 2022

edwardmack deleted the ed/create_dot_unit_tests branch June 21, 2022 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testing (dot) create dot unit tests #1980

testing (dot) create dot unit tests #1980

edwardmack commented Nov 3, 2021 •

edited

Loading

qdm12 commented Nov 4, 2021

edwardmack commented Nov 5, 2021

Other general comments

codecov bot commented Nov 5, 2021 •

edited

Loading

qdm12 commented Nov 6, 2021

timwu20 commented Nov 8, 2021

timwu20 Nov 8, 2021

edwardmack Nov 12, 2021

timwu20 commented Nov 8, 2021

qdm12 Nov 10, 2021

edwardmack Nov 12, 2021

qdm12 Nov 11, 2021

edwardmack Nov 12, 2021

qdm12 Nov 11, 2021

edwardmack Nov 12, 2021

qdm12 Nov 11, 2021

qdm12 Nov 11, 2021

edwardmack Nov 12, 2021

noot Dec 15, 2021

noot Dec 15, 2021

noot Dec 15, 2021

noot Dec 15, 2021

noot Dec 15, 2021

noot Dec 15, 2021

noot left a comment

edwardmack commented Jan 11, 2022

	//InitKeystore to initialize node keystore
	// InitKeystore initialised the node's keystore

testing (dot) create dot unit tests #1980

testing (dot) create dot unit tests #1980

Conversation

edwardmack commented Nov 3, 2021 • edited Loading

Changes

Tests

Issues

Primary Reviewer

qdm12 commented Nov 4, 2021

Other general comments

edwardmack commented Nov 5, 2021

Other general comments

codecov bot commented Nov 5, 2021 • edited Loading

Codecov Report

qdm12 commented Nov 6, 2021

timwu20 commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timwu20 commented Nov 8, 2021

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

noot left a comment

Choose a reason for hiding this comment

edwardmack commented Jan 11, 2022

edwardmack commented Nov 3, 2021 •

edited

Loading

codecov bot commented Nov 5, 2021 •

edited

Loading