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

Coverage for: dataRetriever #5213

Merged
merged 9 commits into from May 26, 2023
Merged

Coverage for: dataRetriever #5213

merged 9 commits into from May 26, 2023

Conversation

sstanculeanu
Copy link
Contributor

@sstanculeanu sstanculeanu commented May 3, 2023

Reasoning behind the pull request

  • increase code coverage

Proposed changes

  • added unittests for dataRetriever

Testing procedure

  • unit tests only

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?

@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.37 🎉

Comparison is base (341e430) 79.41% compared to head (ce62550) 79.79%.

❗ Current head ce62550 differs from pull request most recent head 0a981e6. Consider uploading reports for the commit 0a981e6 to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##           rc/v1.6.0    #5213      +/-   ##
=============================================
+ Coverage      79.41%   79.79%   +0.37%     
=============================================
  Files            691      691              
  Lines          89197    89197              
=============================================
+ Hits           70836    71171     +335     
+ Misses         13165    12894     -271     
+ Partials        5196     5132      -64     
Impacted Files Coverage Δ
...ataRetriever/dataPool/headersCache/headersCache.go 90.15% <ø> (+10.60%) ⬆️

... and 26 files with indirect coverage changes

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

@iulianpascalau iulianpascalau self-requested a review May 3, 2023 17:22
@@ -81,3 +82,14 @@ func TestBlockChain_SettersAndGettersNilValues(t *testing.T) {
assert.Nil(t, bc.GetCurrentBlockHeader())
assert.Empty(t, bc.GetCurrentBlockRootHash())
}

func TestBlockChain_SettersInvalidValues(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -81,6 +82,7 @@ func testRequestDataFromHashArray(t *testing.T, requesterType requestHandlerType
t.Run("should work", func(t *testing.T) {
t.Parallel()

_ = logger.SetLogLevel("*:TRACE") // coverage
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -308,3 +312,50 @@ func TestShardedData_SearchFirstDataFoundShouldRetResults(t *testing.T) {
}

// TODO: Add high load test, reach maximum capacity and inspect RAM usage. EN-6735.
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO still relevant? cc @andreibancioiu

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not relevant anymore. The "memory tests" partially serve this TODO.

TODO can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

iulianpascalau
iulianpascalau previously approved these changes May 4, 2023
@bogdan-rosianu bogdan-rosianu self-requested a review May 9, 2023 13:00
Copy link
Contributor

@bogdan-rosianu bogdan-rosianu left a comment

Choose a reason for hiding this comment

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

typos on the following lines:

  • dataRetriever/factory/containers/resolversContainer_test.go:295
  • dataRetriever/factory/containers/resolversContainer_test.go:272
  • dataRetriever/factory/containers/resolversContainer_test.go:254
  • dataRetriever/shardedData/shardedData_test.go:231
  • dataRetriever/shardedData/shardedData_test.go:81
  • dataRetriever/resolvers/validatorInfoResolver_test.go:382
  • dataRetriever/interface.go:319
  • dataRetriever/topicSender/diffPeerListCreator_test.go:78
  • dataRetriever/factory/epochProviders/currentEpochProvidersFactory_test.go:28
  • dataRetriever/mock/marshalizerMock.go:35

err := hdReq.RequestDataFromEpoch(epochIdentifier)
assert.Equal(t, core.ErrInvalidIdentifierForEpochStartBlockRequest, err)
})
t.Run("identifier not found should error should error", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

"should error should error"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed

bogdan-rosianu
bogdan-rosianu previously approved these changes May 9, 2023
iulianpascalau
iulianpascalau previously approved these changes May 9, 2023
@ssd04 ssd04 self-requested a review May 25, 2023 14:39
@sstanculeanu sstanculeanu merged commit 4c49a65 into rc/v1.6.0 May 26, 2023
5 checks passed
@sstanculeanu sstanculeanu deleted the dataretriever_tests branch May 26, 2023 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants