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: factory/heartbeat, factory/network, factory/state #5152

Merged
merged 14 commits into from Apr 25, 2023

Conversation

sstanculeanu
Copy link
Contributor

@sstanculeanu sstanculeanu commented Apr 4, 2023

Reasoning behind the pull request

  • increase code coverage

Proposed changes

  • added unittests for factory/heartbeat, factory/network, factory/state

Testing procedure

  • standard system test

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?

@sstanculeanu sstanculeanu marked this pull request as draft April 4, 2023 13:01
@sstanculeanu sstanculeanu changed the title [DO NOT MERGE YET] Coverage for: factory/heartbeat, factory/network [DO NOT MERGE YET] Coverage for: factory/heartbeat, factory/network, factory/state Apr 4, 2023
@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Patch coverage: 95.34% and project coverage change: +0.33 🎉

Comparison is base (162d55e) 77.75% compared to head (d89d8c5) 78.08%.

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

Additional details and impacted files
@@              Coverage Diff              @@
##           rc/v1.6.0    #5152      +/-   ##
=============================================
+ Coverage      77.75%   78.08%   +0.33%     
=============================================
  Files            680      680              
  Lines          88348    88360      +12     
=============================================
+ Hits           68694    68997     +303     
+ Misses         14313    14085     -228     
+ Partials        5341     5278      -63     
Impacted Files Coverage Δ
node/nodeRunner.go 73.94% <ø> (-0.05%) ⬇️
factory/bootstrap/bootstrapComponents.go 91.89% <50.00%> (+18.91%) ⬆️
factory/bootstrap/shardingFactory.go 93.42% <100.00%> (+79.45%) ⬆️
factory/heartbeat/heartbeatV2Components.go 98.89% <100.00%> (+24.45%) ⬆️
factory/heartbeat/heartbeatV2ComponentsHandler.go 90.69% <100.00%> (+24.84%) ⬆️
factory/state/stateComponents.go 79.72% <100.00%> (+12.02%) ⬆️
storage/factory/openStorage.go 83.50% <100.00%> (+2.85%) ⬆️
storage/latestData/latestDataProvider.go 77.24% <100.00%> (+0.55%) ⬆️

... and 9 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@sstanculeanu sstanculeanu marked this pull request as ready for review April 6, 2023 13:03
@SebastianMarian SebastianMarian self-requested a review April 10, 2023 15:47
@ssd04 ssd04 self-requested a review April 12, 2023 06:36
if check.IfNil(args.StorageService) {
return nil, errors.ErrNilStorageService
}
if check.IfNil(args.ChainHandler) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed to check for nil ChainHandler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only kept checks on the components that are used inside the factory and removed the ones that are only forwarded to subcomponents(which should do their checks on the received params)

example:

if check.IfNil(args.StatusCoreComponents) {
return errors.ErrNilStatusCoreComponents
}
if check.IfNil(args.StatusCoreComponents.AppStatusHandler()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed to check for nil AppStatusHandler and HardforkTrigger ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above, not used inside the heartbeatV2Components, only forwarded

Comment on lines +62 to +63
coreCompStub.InternalMarshalizerCalled = func() marshal.Marshalizer {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

so this will make CreateTriesComponentsForShardId fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes CreateTriesComponentsForShardId passes the marshaller on NewTrieFactory which will eventually fail

Comment on lines 230 to 231
t.Run("nil EpochStartTrigger should error", func(t *testing.T) {
t.Parallel()
Copy link
Contributor

Choose a reason for hiding this comment

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

this inner test looks to be duplicated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, removed one

ssd04
ssd04 previously approved these changes Apr 13, 2023
gabi-vuls
gabi-vuls previously approved these changes Apr 19, 2023
Copy link
Collaborator

@gabi-vuls gabi-vuls left a comment

Choose a reason for hiding this comment

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

System test passed
@@ Log scanner @@

more_factory_tests_p2

================================================================================

  • Known Warnings 9
  • New Warnings 2
  • Known Errors 0
  • New Errors 0
  • Panics 0
    ================================================================================
  • block hash does not match 8444
  • wrong nonce in block 3410
  • miniblocks does not match 0
  • num miniblocks does not match 0
  • miniblock hash does not match 0
  • block bodies does not match 0
  • receipts hash missmatch 0
    ================================================================================
  • No jailed nodes on the testnet
    ================================================================================

@sstanculeanu sstanculeanu changed the base branch from rc/v1.5.0 to rc/v1.6.0 April 24, 2023 16:56
@sstanculeanu sstanculeanu dismissed stale reviews from gabi-vuls and ssd04 April 24, 2023 16:56

The base branch was changed.

@sstanculeanu sstanculeanu changed the title [DO NOT MERGE YET] Coverage for: factory/heartbeat, factory/network, factory/state Coverage for: factory/heartbeat, factory/network, factory/state Apr 24, 2023
…into more_factory_tests_p2

# Conflicts:
#	factory/heartbeat/heartbeatV2Components_test.go
@sstanculeanu sstanculeanu merged commit 40c3887 into rc/v1.6.0 Apr 25, 2023
5 checks passed
@sstanculeanu sstanculeanu deleted the more_factory_tests_p2 branch April 25, 2023 09:31
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

4 participants