Prepare Bor package for testing by JekaMas · Pull Request #416 · 0xPolygon/bor

JekaMas · 2022-05-26T13:58:38Z

As a developer I'd like to test Bor consensus. However, it is tightly coupled with a few dependancies, so it is hard to test.
This PR extracts dependancies introducing interfaces

codecov-commenter · 2022-05-27T06:45:50Z

Codecov Report

Merging #416 (200d62e) into v0.3.0-dev (2323f2c) will increase coverage by 0.73%.
The diff coverage is 30.08%.

@@              Coverage Diff               @@
##           v0.3.0-dev     #416      +/-   ##
==============================================
+ Coverage       55.74%   56.48%   +0.73%     
==============================================
  Files             601      589      -12     
  Lines           69602    68434    -1168     
==============================================
- Hits            38803    38656     -147     
+ Misses          27447    26465     -982     
+ Partials         3352     3313      -39

Impacted Files	Coverage Δ
consensus/bor/api.go	`0.00% <0.00%> (ø)`
consensus/bor/bor.go	`7.72% <0.00%> (+1.49%)`	⬆️
consensus/bor/errors.go	`0.00% <0.00%> (ø)`
consensus/bor/genesis_contract_mock.go	`0.00% <0.00%> (ø)`
consensus/bor/merkle.go	`0.00% <0.00%> (ø)`
consensus/bor/snapshot.go	`14.77% <0.00%> (+0.94%)`	⬆️
consensus/bor/span_mock.go	`0.00% <0.00%> (ø)`
consensus/bor/validators_getter_mock.go	`0.00% <0.00%> (ø)`
core/blockchain_reader.go	`43.44% <0.00%> (-3.23%)`	⬇️
core/chain_makers.go	`68.21% <0.00%> (-1.87%)`	⬇️
... and 80 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 4ffdbfe...200d62e. Read the comment docs.

temaniarpit27 · 2022-05-31T12:04:35Z

@JekaMas can we split this PR somehow?
difficult to review all at once due to the number of files changed

JekaMas · 2022-05-31T12:07:05Z

@JekaMas can we split this PR somehow? difficult to review all at once due to the number of files changed

@temaniarpit27 Unfortunately, I can't find the way to split it.
I'd suggest reviewing with focus on features - start with bor package first.

temaniarpit27 · 2022-05-31T18:32:10Z

One thing
Can we move all the tests to one place?
snapshot_test and if there are any other test files to tests/bor?

JekaMas · 2022-05-31T18:35:29Z

One thing Can we move all the tests to one place? snapshot_test and if there are any other test files to tests/bor?

Why you would like to do it? My arguments to keep things as they are:

To be able to write unit tests to private functions/methods
To make it obvious to what files we have tests (tests are in the same package)
To separate integration and unit level tests
It is common golang practice

cffls

Looks good to me overall. My only concern is that with lots of refactoring in this PR, it is not obviously easy to detect problems and bugs by simply reading the code. What testings do you think we could perform to give us more confidence in merging this change?

cffls · 2022-06-01T07:21:06Z

@@ -0,0 +1,11 @@
+package set
+
+func New[T comparable](slice []T) map[T]struct{} {


Where is this function used?

Looks good to me overall. My only concern is that with lots of refactoring in this PR, it is not obviously easy to detect problems and bugs by simply reading the code. What testings do you think we could perform to give us more confidence in merging this change?

I believe the only change in bor package is interaction between bor and Heimdall. So we need to test: getting new spans, state sync.

Where is this function used?

Yes, it is being used in TestRandomAddresses

I believe the only change in bor package is interaction between bor and Heimdall. So we need to test: getting new spans, state sync.

We can create a local devnet to test this out.

It'd be great to test heimdall+bor on devnet. Check that blocks, checkpoints, state sync, spans are there and keep going.

Could you take testing on you? Just basic one. If no errors I believe we can move further with the task and merge it. @cffls

Sure, I will take this up.

@cffls does it looks good to you in the current state?

Yes, the local devnet looks good after using the latest commits.

I believe we can merge it now.

temaniarpit27 · 2022-06-01T16:32:54Z

Most of it looks fine to me

ferranbt and others added 12 commits May 26, 2022 16:55

Limit state sync by gas

ca073e0

Added logging for state-sync total gas usage

7641eea

Added number of event-records in log

9a26fa7

Minor Changes

2112d4f

Minor Fix

dd6ea04

Adding individual gasUsed

620420f

Minor Fix

fc8be1c

it works

efba846

fix tests

8c6b73f

log wiggle and delay with block number

c5692ce

log delays as numbers

df06549

merge v0.3.0-dev

397860f

JekaMas requested review from 0xKrishna, cffls, manav2401 and paulgoleary May 27, 2022 06:01

JekaMas added 2 commits May 27, 2022 09:14

linters

7ffc828

fix tests

f7b43d8

JekaMas added 8 commits May 27, 2022 11:27

restore linters for the project

b4df062

fix linters

241e258

fix

4c9bc70

fix

02c8d6d

fix

89dd1c6

linters

ad5e4f5

generation

48f52f9

fix tests

c8f9cd6

JekaMas force-pushed the prepare-bor-for-testing branch from f179c86 to c8f9cd6 Compare May 30, 2022 14:15

JekaMas added 2 commits May 30, 2022 17:21

remove heimdall wrapper response

46f17a5

linters

11f9be8

JekaMas added 5 commits May 30, 2022 18:07

remove possible collisions

0a697ef

remove possible collisions

a3c2fd4

remove possible collisions

a065083

tests for unique address generation

8143b1d

generalize set

20a265d

temaniarpit27 reviewed May 31, 2022

View reviewed changes

Comment thread Makefile Outdated

temaniarpit27 reviewed May 31, 2022

View reviewed changes

Comment thread internal/cli/server/chains/chain_test.go

temaniarpit27 reviewed May 31, 2022

View reviewed changes

Comment thread go.mod

temaniarpit27 reviewed May 31, 2022

View reviewed changes

Comment thread consensus/bor/set/error.go Outdated

cffls reviewed Jun 1, 2022

View reviewed changes

JekaMas added 2 commits June 1, 2022 13:56

bor miner tests got restored

cae92d5

fixes after CR

73dfc1d

temaniarpit27 reviewed Jun 1, 2022

View reviewed changes

Comment thread consensus/bor/bor.go

JekaMas added 7 commits June 2, 2022 19:25

final step and mining test

6301086

fix

4ef062c

Merge branch 'v0.3.0-dev' into prepare-bor-for-testing

f00c0c1

fix e2e

200d62e

more tests for Heimdall requests

072f9ed

Merge branch 'v0.3.0-dev' into prepare-bor-for-testing

1d5f737

fix linters

720e93f

temaniarpit27 approved these changes Jun 8, 2022

View reviewed changes

0xKrishna approved these changes Jun 8, 2022

View reviewed changes

JekaMas merged commit 9c8bf51 into v0.3.0-dev Jun 8, 2022

JekaMas deleted the prepare-bor-for-testing branch June 8, 2022 13:39

		@@ -0,0 +1,11 @@
		package set

		func New[T comparable](slice []T) map[T]struct{} {

Conversation

JekaMas commented May 26, 2022

Uh oh!

codecov-commenter commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

temaniarpit27 commented May 31, 2022

Uh oh!

JekaMas commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

temaniarpit27 commented May 31, 2022

Uh oh!

JekaMas commented May 31, 2022

Uh oh!

cffls left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JekaMas Jun 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

temaniarpit27 commented Jun 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-commenter commented May 27, 2022 •

edited

Loading

JekaMas commented May 31, 2022 •

edited

Loading

JekaMas Jun 3, 2022 •

edited

Loading