Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(dot/sync): simplify processBlockData and associated tests #2811

Merged
merged 21 commits into from
Nov 15, 2022

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Sep 8, 2022

Changes

  • processBlockData:
    • Split subblocks into functions:
      • processBlockDataWithHeaderAndBody function with test
      • processBlockDataWithStateHeaderAndBody function with test
    • takes block data blockData as value argument instead of pointer
    • Review error wrappings
    • Remove block data nil check
    • Rename receiver from s to c for chainProcessor
  • Remove unneeded unit test cases for processBlockData (now covered in subfunctions associated tests)
  • Push (possibly unneeded) justification length check up in caller of handleJustification.
  • Return error on block import error
  • Fix position of debug logs

Done whilst fixing tests in #2774

🆕 is kinda blocking me on #2912 (trying to fix the memmm)

Tests

go test -tags integration github.com/ChainSafe/gossamer/dot/sync

Issues

Primary Reviewer

@edwardmack

@qdm12 qdm12 changed the base branch from development to qdm12/dot/sync/handle-just September 8, 2022 19:06
@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #2811 (2de6f19) into development (b2a8a86) will increase coverage by 0.07%.
The diff coverage is 98.19%.

Additional details and impacted files
@@               Coverage Diff               @@
##           development    #2811      +/-   ##
===============================================
+ Coverage        63.40%   63.47%   +0.07%     
===============================================
  Files              218      218              
  Lines            27496    27505       +9     
===============================================
+ Hits             17434    17460      +26     
+ Misses            8461     8442      -19     
- Partials          1601     1603       +2     

@qdm12 qdm12 force-pushed the qdm12/dot/sync/handle-just branch 2 times, most recently from ceb798e to 1d661d6 Compare September 16, 2022 12:29
@qdm12 qdm12 force-pushed the qdm12/dot/sync/handle-just branch 2 times, most recently from 793bebc to 7707152 Compare September 22, 2022 17:40
Base automatically changed from qdm12/dot/sync/handle-just to development September 22, 2022 18:15
@qdm12 qdm12 force-pushed the qdm12/dot/sync/processblockdata branch 2 times, most recently from e4d3ce8 to b3c84a1 Compare October 3, 2022 07:41
dot/sync/chain_processor.go Show resolved Hide resolved
dot/sync/chain_processor_test.go Show resolved Hide resolved
@qdm12 qdm12 marked this pull request as ready for review October 3, 2022 07:48
@qdm12 qdm12 marked this pull request as draft October 3, 2022 12:44
@qdm12 qdm12 force-pushed the qdm12/dot/sync/processblockdata branch from b1dbb04 to 81c1568 Compare October 4, 2022 13:09
@qdm12 qdm12 marked this pull request as ready for review October 4, 2022 13:12
dot/sync/chain_processor.go Show resolved Hide resolved
dot/sync/chain_processor.go Outdated Show resolved Hide resolved
dot/sync/chain_processor.go Outdated Show resolved Hide resolved
@qdm12 qdm12 force-pushed the qdm12/dot/sync/processblockdata branch from 81c1568 to 755c22c Compare October 6, 2022 16:00
Copy link
Member

@EclesioMeloJunior EclesioMeloJunior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@qdm12 qdm12 force-pushed the qdm12/dot/sync/processblockdata branch from 755c22c to b00715a Compare November 3, 2022 10:08
Copy link
Contributor

@q9f q9f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot review the logic but left some questions and comments regarding style :)

dot/sync/chain_processor.go Outdated Show resolved Hide resolved
dot/sync/chain_processor.go Show resolved Hide resolved
dot/sync/chain_processor.go Show resolved Hide resolved
dot/sync/chain_processor.go Show resolved Hide resolved
dot/sync/chain_processor.go Show resolved Hide resolved
dot/sync/chain_processor.go Outdated Show resolved Hide resolved
@qdm12 qdm12 force-pushed the qdm12/dot/sync/processblockdata branch 2 times, most recently from 917f5ca to 354d4c7 Compare November 7, 2022 16:41
dot/sync/chain_processor.go Outdated Show resolved Hide resolved
@qdm12 qdm12 force-pushed the qdm12/dot/sync/processblockdata branch 2 times, most recently from 56ab6ce to e2429de Compare November 9, 2022 13:46
@qdm12 qdm12 force-pushed the qdm12/dot/sync/processblockdata branch from e2429de to 8c5bac2 Compare November 9, 2022 16:41
@qdm12 qdm12 force-pushed the qdm12/dot/sync/processblockdata branch from 8c5bac2 to eab9165 Compare November 15, 2022 11:20
@qdm12 qdm12 force-pushed the qdm12/dot/sync/processblockdata branch from eab9165 to 2de6f19 Compare November 15, 2022 15:08
@qdm12 qdm12 merged commit 8a17f37 into development Nov 15, 2022
@qdm12 qdm12 deleted the qdm12/dot/sync/processblockdata branch November 15, 2022 16:59
@github-actions
Copy link

🎉 This PR is included in version 0.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants