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

unit tests for state/syncer #5239

Merged
merged 4 commits into from
May 15, 2023

Conversation

ssd04
Copy link
Contributor

@ssd04 ssd04 commented May 12, 2023

Reasoning behind the pull request

  • Add unit tests before making changes in trie sync process

Proposed changes

  • Added unit tests for state/syncer package

Testing procedure

  • N/A

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?

@ssd04 ssd04 self-assigned this May 12, 2023
@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

❗ No coverage uploaded for pull request base (feat/trie-sync-optimizations@ac49dc0). Click here to learn what that means.
Patch has no changes to coverable lines.

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

Additional details and impacted files
@@                       Coverage Diff                       @@
##             feat/trie-sync-optimizations    #5239   +/-   ##
===============================================================
  Coverage                                ?   79.08%           
===============================================================
  Files                                   ?      683           
  Lines                                   ?    88367           
  Branches                                ?        0           
===============================================================
  Hits                                    ?    69887           
  Misses                                  ?    13273           
  Partials                                ?     5207           

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

@ssd04 ssd04 marked this pull request as ready for review May 12, 2023 12:41
@ssd04 ssd04 changed the title unit tests for constructors in syncer unit tests for state/syncer May 12, 2023
@BeniaminDrasovean BeniaminDrasovean self-requested a review May 12, 2023 12:46
@sstanculeanu sstanculeanu self-requested a review May 12, 2023 12:50
return tr
}

func TestUserAccountsSyncer_SyncerAccountDataTries(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
func TestUserAccountsSyncer_SyncerAccountDataTries(t *testing.T) {
func TestUserAccountsSyncer_SyncAccountDataTries(t *testing.T) {

require.Equal(t, expectedErr, err)
})

t.Run("should work", func(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

few notes here in order to increase the coverage:

  1. missing IsInterfaceNil test
  2. coverage can be increased with a test similar with should work, but throttler.CanProcess returns false and SyncAccountDataTries gets as param a closed context(this would lead to time is out error)
  3. a new test with ReadFromChanNonBlocking returning error
  4. perhaps a test with largeTries, to cover more of printDataTrieStatistics(not sure if doable)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added more tests, might add for more scenarios in a later task

"github.com/stretchr/testify/require"
)

func TestNewValidatorAccountsSyncer(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing IsInterfaceNil test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

Copy link
Contributor

@BeniaminDrasovean BeniaminDrasovean left a comment

Choose a reason for hiding this comment

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

Nothing from me, just the tests that Sorin suggested.

@ssd04 ssd04 merged commit 5a1414d into feat/trie-sync-optimizations May 15, 2023
@ssd04 ssd04 deleted the state-syncer-unit-tests branch May 15, 2023 12:57
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.

3 participants