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

Peers rating handler integration test #4781

Conversation

sstanculeanu
Copy link
Contributor

Reasoning behind the pull request

  • an integration test is needed to properly test peers rating handler

Proposed changes

  • added integration test and updated elrond-go-p2p

Testing procedure

  • integration 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?

… on peersRatingHandler and added integration test for peersRatingHandler
SebastianMarian
SebastianMarian previously approved these changes Dec 8, 2022
@sstanculeanu sstanculeanu marked this pull request as ready for review December 9, 2022 17:16
@sstanculeanu sstanculeanu marked this pull request as draft December 13, 2022 13:05
Base automatically changed from fix_peers_rating_handler to feat/fix-peers-rating-handler December 13, 2022 13:09
…nto peers_rating_handler_integration_test

# Conflicts:
#	go.mod
#	go.sum
#	integrationTests/testProcessorNode.go
#	node/metrics/metrics.go
@sstanculeanu sstanculeanu marked this pull request as ready for review December 13, 2022 13:16
Copy link
Contributor

@iulianpascalau iulianpascalau left a comment

Choose a reason for hiding this comment

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

👍 GJ

assert.Equal(t, int32(-100), initialMaliciousRating.Rating)
testTimestampsForNotRespondingNode(t, initialMaliciousRating.TimestampLastResponseFromPid, initialMaliciousRating.TimestampLastRequestToPid, 0)

// Add header to the malicious node's cache and remove it from the resolver's cache
Copy link
Contributor

Choose a reason for hiding this comment

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

L98 - L116 also test that requests are still performed towards the low rating peer 👍

})
appStatusHandler := args.AppStatusHandler
if check.IfNil(args.AppStatusHandler) {
appStatusHandler = TestAppStatusHandler
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this OK? Having the same pointer TestAppStatusHandler will make debugging incredibly hard because nodes will tend to alter the same instance. Maybe not for this PR I will consider removing this TestAppStatusHandler and create different instances always.

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, shouldn't use the same pointer

@sstanculeanu sstanculeanu merged commit 54711c2 into feat/fix-peers-rating-handler Dec 13, 2022
@sstanculeanu sstanculeanu deleted the peers_rating_handler_integration_test branch December 13, 2022 14:27
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

3 participants