testing(dot): create dot unit tests #2112

edwardmack · 2021-12-10T22:33:21Z

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

codecov · 2021-12-10T23:04:17Z

Codecov Report

Merging #2112 (d894db6) into development (ded3a0b) will increase coverage by 3.22%.
The diff coverage is 48.55%.

@@               Coverage Diff               @@
##           development    #2112      +/-   ##
===============================================
+ Coverage        58.50%   61.73%   +3.22%     
===============================================
  Files              214      216       +2     
  Lines            28182    28179       -3     
===============================================
+ Hits             16489    17395     +906     
+ Misses           10037     9039     -998     
- Partials          1656     1745      +89

Flag	Coverage Δ
unit-tests	`61.73% <48.55%> (+3.22%)`	⬆️

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

Impacted Files	Coverage Δ
dot/state/service.go	`49.55% <0.00%> (-0.68%)`	⬇️
dot/telemetry/telemetry.go	`0.00% <0.00%> (ø)`
lib/babe/babe.go	`3.00% <0.00%> (-0.58%)`	⬇️
lib/services/services.go	`81.39% <ø> (ø)`
dot/utils.go	`33.33% <23.07%> (+33.33%)`	⬆️
internal/trie/node/decode.go	`96.07% <55.55%> (-3.93%)`	⬇️
dot/services.go	`73.59% <60.00%> (+73.59%)`	⬆️
dot/node.go	`56.21% <70.76%> (+56.21%)`	⬆️
lib/trie/database.go	`52.13% <82.35%> (+1.17%)`	⬆️
dot/build_spec.go	`80.58% <100.00%> (+80.58%)`	⬆️
... and 12 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 9efde47...d894db6. Read the comment docs.

dot/build_spec_test.go

qdm12 · 2021-12-13T12:52:06Z

dot/build_spec_test.go

+			if tt.want != nil {
+				assert.Equal(t, tt.want.genesis.Name, got.genesis.Name)
+			}


Can you assert the entire BuildSpec here 🤔 ?

I don't check the entire BuildSpec because it would require comparing the code entry which is very large.

Maybe just force set the code field to empty for the actual value and assert the rest?

Like

actual := ... actual.code = nil expected := &BuildSpec{...} assert.Equal(t, expected, actual)

can we resolve this or does this need to updated?

Updated to force set genesis Raw and Runtime fields before comparing.

dot/build_spec_test.go

qdm12 · 2021-12-13T12:54:05Z

dot/build_spec_test.go

+			if tt.want != nil {
+				assert.Equal(t, tt.want.genesis.Name, got.genesis.Name)
+			}


Can you assert the entire buildSpec? Or is there some random aspect to it? Or set unexported fields?

can we resolve this or does this need to updated?

Updated to test buildSpec.

dot/build_spec_test.go

dot/config_test.go

dot/core/service.go

dot/build_spec_test.go

dot/core/service.go

dot/import_test.go

dot/node.go

timwu20 · 2022-01-18T04:33:10Z

@edwardmack this branch is failing to compile.

edwardmack · 2022-01-25T22:08:17Z

I've cleaned up conflicts with this, it should be ready for re-review.

dot/config_integration_test.go

lib/blocktree/blocktree.go

noot

basically looks good just a few comments!

timwu20

TestNewTestGenesisAndRuntime is failing when I run this locally.

edwardmack · 2022-02-02T22:27:25Z

@timwu20 TestNewTestGenesisAndRuntime is working for me locally, please let me know what error you are seeing and I'll try to fix it.

timwu20 · 2022-02-03T23:03:10Z

@timwu20 TestNewTestGenesisAndRuntime is working for me locally, please let me know what error you are seeing and I'll try to fix it.

=== RUN   TestNewTestGenesisAndRuntime
=== RUN   TestNewTestGenesisAndRuntime/working_example
    /Users/timothywu/dev/gossamer/dot/utils_test.go:328:
        	Error Trace:	utils_test.go:328
        	Error:      	Should be true
        	Test:       	TestNewTestGenesisAndRuntime/working_example
--- FAIL: TestNewTestGenesisAndRuntime (1.90s)
    --- FAIL: TestNewTestGenesisAndRuntime/working_example (1.81s)
FAIL
FAIL	github.com/ChainSafe/gossamer/dot	2.514s

Looks like on MacOS it writes to a different folder than /tmp. My path is /var/folders/16/fwrrm5ss4gb6f5d7j6przgf80000gn/T/TestNewTestGenesisAndRuntime3015430054/004/genesis.json

timwu20

Can you move all the NewTestXXX functions into a test file. All the references to these functions are only in test files. Then you don't need to write unit tests for them 😉

edwardmack · 2022-02-04T21:15:52Z

@timwu20 TestNewTestGenesisAndRuntime is working for me locally, please let me know what error you are seeing and I'll try to fix it.
=== RUN   TestNewTestGenesisAndRuntime
=== RUN   TestNewTestGenesisAndRuntime/working_example
    /Users/timothywu/dev/gossamer/dot/utils_test.go:328:
        	Error Trace:	utils_test.go:328
        	Error:      	Should be true
        	Test:       	TestNewTestGenesisAndRuntime/working_example
--- FAIL: TestNewTestGenesisAndRuntime (1.90s)
    --- FAIL: TestNewTestGenesisAndRuntime/working_example (1.81s)
FAIL
FAIL	github.com/ChainSafe/gossamer/dot	2.514s
Looks like on MacOS it writes to a different folder than /tmp. My path is /var/folders/16/fwrrm5ss4gb6f5d7j6przgf80000gn/T/TestNewTestGenesisAndRuntime3015430054/004/genesis.json

I've updated this test.

edwardmack · 2022-02-04T21:16:16Z

Can you move all the NewTestXXX functions into a test file. All the references to these functions are only in test files. Then you don't need to write unit tests for them wink

Good point, I've moved these.

cmd/gossamer/config_test.go

edwardmack · 2022-03-28T21:37:26Z

I've created a new PR #2446, to replace this PR. The new PR has been cleaned-up to show the files that were actually updated for this PR. I believe all the comments for this have been addressed, please see review the new PR.

qdm12

@edwardmack there are still a few unresolved comments I read through above.

I also took the liberty to force push the squashed commits from your new branch from #2112 to this PR's branch so Github keeps track of already reviewed changes, to avoid re-reviewing 4k+ lines from scratch for me and other reviewers.

FYI Github changes web view works by comparing squashed changes vs the first commit the branch has branched out off. That means that rewriting history (like force pushing squashed commits here) is fine and won't affect reviewers changes viewed.

qdm12 · 2022-03-29T08:53:31Z

dot/node_integration_test.go

+	go func() {
+		<-node.started
+		node.Stop()
+	}()
 	err = node.Start()
 	require.NoError(t, err)
-	<-node.started
-	node.Stop()


Maybe it was better before? Start launches a goroutine to handle the node I think, so it should be fine to call all this synchronously. Plus if the start fails and returns a start error, node.started will never get closed

It seems that node.Start() blocks until node.Stop is called. There is a WaitGroup in start that waits until Done is called in Stop, so I needed to put the <-node.started in a routine before calling node.Start because this test was getting stuck never moving past start.

Ooof yeah weird piece of code there. It's a bit out of scope, but maybe rename it to Run() instead of Start()? Since it doesn't really start and return, it 'runs' all the services in a blocking way.

dot/state/service.go

go.mod

qdm12 · 2022-03-29T09:12:25Z

Older unresolved comments:

edwardmack · 2022-03-29T18:59:29Z

I've commented and/or addressed remaining comments, let me know if there are remaining issues.

timwu20

good work Ed!

kishansagathiya · 2022-03-30T05:31:47Z

lib/babe/babe.go

+	NewServiceIFace(cfg *ServiceConfig) (ServiceIFace, error)
+}
+
+var _ ServiceBuilder = (*Builder)(nil)


Why are you using ServiceBuilder?

Why is using ServiceBuilder better than using NewServiceIFace directly?

I think you should be able to using NewServiceIFace directly

The ServiceBuilder interface was created so that we have an interface for generating mocks for that, mocks for this interface are used in service_test.go Test_nodeBuilder_createBABEService tests. I'm not sure how to generate mocks without an interface.

You could have used an anonymous function signature instead of an interface. But you would have to manually write a dummy function that implemented that function signature for testing.

I don't think this interface needs to be defined in the babe package. You could move this interface to be defined in dot to localise it to nodeBuilder.createBABEServiceWithBuilder. It doesn't really make sense for the babe package to have to have an interface for one if its types constructors. Can we change this?

@edwardmack Have you addressed this?

lib/babe/babe.go

dot/utils_test.go

dot/utils_integration_test.go

dot/services.go

dot/build_spec_test.go

dot/errors.go

timwu20

just the moving of the ServiceBuilder interface to where it's required.

timwu20 · 2022-03-30T19:33:54Z

lib/babe/babe.go

+	NewServiceIFace(cfg *ServiceConfig) (ServiceIFace, error)
+}
+
+var _ ServiceBuilder = (*Builder)(nil)


You could have used an anonymous function signature instead of an interface. But you would have to manually write a dummy function that implemented that function signature for testing.

I don't think this interface needs to be defined in the babe package. You could move this interface to be defined in dot to localise it to nodeBuilder.createBABEServiceWithBuilder. It doesn't really make sense for the babe package to have to have an interface for one if its types constructors. Can we change this?

lib/babe/babe.go

edwardmack · 2022-03-30T20:05:26Z

I've updated to address comments, and moved ServiceBuilder interface to dot package. Should be ready for re-re-review.

kishansagathiya · 2022-03-31T23:20:51Z

dot/services_test.go

+			got, err := builder.createSystemService(tt.args.cfg, tt.args.service)
+			assert.ErrorIs(t, err, tt.err)
+
+			// TODO: change this check to assert.Equal after state.Service interface is implemented.


There are five TODO statements in this file, remove when applicable

edwardmack requested review from arijitAD, EclesioMeloJunior, jimjbrettj, kishansagathiya, noot, qdm12 and timwu20 as code owners December 10, 2021 22:33

edwardmack self-assigned this Dec 10, 2021

edwardmack added the Status: Review Needed label Dec 10, 2021

qdm12 reviewed Dec 13, 2021

View reviewed changes

timwu20 requested changes Dec 14, 2021

View reviewed changes

edwardmack mentioned this pull request Jan 11, 2022

testing (dot) create dot unit tests #1980

Closed

edwardmack force-pushed the ed/dot_tests_to_integration branch from bed245e to 2a4db58 Compare January 21, 2022 20:55

edwardmack force-pushed the ed/create_dot_unit_tests_b branch 2 times, most recently from 46dd3ab to a2bebe1 Compare January 25, 2022 21:46

noot reviewed Jan 26, 2022

View reviewed changes

dot/config_integration_test.go Outdated Show resolved Hide resolved

ChainSafe deleted a comment from edwardmack Jan 26, 2022

noot reviewed Jan 26, 2022

View reviewed changes

lib/blocktree/blocktree.go Outdated Show resolved Hide resolved

noot reviewed Jan 26, 2022

View reviewed changes

timwu20 requested changes Jan 31, 2022

View reviewed changes

timwu20 requested changes Feb 3, 2022

View reviewed changes

noot reviewed Feb 7, 2022

View reviewed changes

cmd/gossamer/config_test.go Outdated Show resolved Hide resolved

noot approved these changes Feb 7, 2022

View reviewed changes

add dot integration test to github workflow

19f788b

removed test no longer relavent

c145175

qdm12 force-pushed the ed/create_dot_unit_tests_b branch from 15993cc to c145175 Compare March 29, 2022 08:45

qdm12 reviewed Mar 29, 2022

View reviewed changes

qdm12 mentioned this pull request Mar 29, 2022

testing(dot): create dot unit tests #2446

Closed

edwardmack added 2 commits March 29, 2022 13:52

address PR comments, assert.Equal sturcts

90e869b

create setConfigTestDefaults func, add wasm check

54e0906

timwu20 approved these changes Mar 29, 2022

View reviewed changes

kishansagathiya requested changes Mar 30, 2022

View reviewed changes

qdm12 reviewed Mar 30, 2022

View reviewed changes

dot/services.go Outdated Show resolved Hide resolved

kishansagathiya reviewed Mar 30, 2022

View reviewed changes

dot/build_spec_test.go Show resolved Hide resolved

dot/build_spec_test.go Outdated Show resolved Hide resolved

use filepath.Join, use a sentinel error, remove un-needed check

7a8b5b1

qdm12 approved these changes Mar 30, 2022

View reviewed changes

dot/errors.go Outdated Show resolved Hide resolved

edwardmack added 2 commits March 30, 2022 12:39

remove un-used test util functions

40506d9

removed un-needed comment

d955f93

timwu20 requested changes Mar 30, 2022

View reviewed changes

move NewServiceIFace to dot package

5ce4f1b

update go generate, add copyright

c6e136d

timwu20 approved these changes Mar 30, 2022

View reviewed changes

edwardmack requested a review from kishansagathiya March 30, 2022 23:13

kishansagathiya approved these changes Mar 31, 2022

View reviewed changes

kishansagathiya reviewed Mar 31, 2022

View reviewed changes

qdm12 mentioned this pull request Apr 1, 2022

chore(cmd/gossamer): exportConfig returns an error instead of calling os.Exit(1) #2461

Merged

Merge branch 'development' into ed/create_dot_unit_tests_b

d894db6

edwardmack merged commit 620535d into development Apr 4, 2022

edwardmack deleted the ed/create_dot_unit_tests_b branch April 4, 2022 16:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

testing(dot): create dot unit tests #2112

testing(dot): create dot unit tests #2112

edwardmack commented Dec 10, 2021

codecov bot commented Dec 10, 2021 •

edited

Loading

qdm12 Dec 13, 2021

edwardmack Jan 27, 2022

qdm12 Feb 12, 2022

timwu20 Mar 15, 2022

edwardmack Mar 29, 2022

qdm12 Dec 13, 2021 •

edited

Loading

timwu20 Mar 15, 2022

edwardmack Mar 29, 2022

timwu20 commented Jan 18, 2022

edwardmack commented Jan 25, 2022

noot left a comment

timwu20 left a comment

edwardmack commented Feb 2, 2022

timwu20 commented Feb 3, 2022 •

edited

Loading

timwu20 left a comment

edwardmack commented Feb 4, 2022

edwardmack commented Feb 4, 2022

edwardmack commented Mar 28, 2022

qdm12 left a comment

qdm12 Mar 29, 2022

edwardmack Mar 29, 2022

qdm12 Mar 29, 2022

qdm12 commented Mar 29, 2022

edwardmack commented Mar 29, 2022

timwu20 left a comment

kishansagathiya Mar 30, 2022

edwardmack Mar 30, 2022

timwu20 Mar 30, 2022 •

edited

Loading

kishansagathiya Mar 31, 2022

timwu20 left a comment

timwu20 Mar 30, 2022 •

edited

Loading

edwardmack commented Mar 30, 2022

kishansagathiya Mar 31, 2022

testing(dot): create dot unit tests #2112

testing(dot): create dot unit tests #2112

Conversation

edwardmack commented Dec 10, 2021

Changes

Tests

Issues

Primary Reviewer

codecov bot commented Dec 10, 2021 • edited 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

qdm12 Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timwu20 commented Jan 18, 2022

edwardmack commented Jan 25, 2022

noot left a comment

Choose a reason for hiding this comment

timwu20 left a comment

Choose a reason for hiding this comment

edwardmack commented Feb 2, 2022

timwu20 commented Feb 3, 2022 • edited Loading

timwu20 left a comment

Choose a reason for hiding this comment

edwardmack commented Feb 4, 2022

edwardmack commented Feb 4, 2022

edwardmack commented Mar 28, 2022

qdm12 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

qdm12 commented Mar 29, 2022

edwardmack commented Mar 29, 2022

timwu20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timwu20 Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timwu20 left a comment

Choose a reason for hiding this comment

timwu20 Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

edwardmack commented Mar 30, 2022

Choose a reason for hiding this comment

codecov bot commented Dec 10, 2021 •

edited

Loading

qdm12 Dec 13, 2021 •

edited

Loading

timwu20 commented Feb 3, 2022 •

edited

Loading

timwu20 Mar 30, 2022 •

edited

Loading

timwu20 Mar 30, 2022 •

edited

Loading