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

blockchain: Test & Check Memory Usage #2167

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

web3santa
Copy link

Use the newUtxoCache function to create a 1MB simulated newUtxoCache instance.

Call the totalMemoryUsage method to compare the returned result to the expected size of 1 MB.

If the result is different from the expected size, the test fails and outputs an error message.

This test code is responsible for ensuring that the memory usage of newUtxoCache matches the expected value.

resultShoulBeTrue := result == expectedSize

// Compare the expected result with the actual result
if resultShoulBeTrue {
Copy link
Contributor

Choose a reason for hiding this comment

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

It appears that there is a logical error in the condition if resultShoulBeTrue. This condition implies that the test should fail when the memory usage matches the expected size, which is the opposite of the intended behavior.

I recommend changing the condition to if !resultShoulBeTrue or if result != expectedSize, indicating that the test should fail when the memory usage does not match the expected size

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, it's important to note that totalMemoryUsage and maxTotalMemoryUsage represent different concepts. maxTotalMemoryUsage is the maximum memory usage allowed for the cache in normal circumstances, while totalMemoryUsage is the map size + the size that the utxo entries are taking up.

Copy link
Contributor

@SulaimanAminuBarkindo SulaimanAminuBarkindo Apr 17, 2024

Choose a reason for hiding this comment

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

Overall, thank you for your contribution to the project and for your efforts in writing tests.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8680316882

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.02%) to 56.886%

Files with Coverage Reduction New Missed Lines %
peer/peer.go 1 74.52%
mempool/mempool.go 1 66.84%
Totals Coverage Status
Change from base Build 8636847188: 0.02%
Covered Lines: 29456
Relevant Lines: 51781

💛 - Coveralls

@kcalvinalvin
Copy link
Collaborator

This is essentially the same test as

func TestCalculateEntries(t *testing.T) {

since there aren't any additional entries being added.

Could you change the tests to include utxo entries so that the test isn't a duplicate?

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.

4 participants