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.

Sign up for GitHub

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

Jump to bottom

chore(trie): lib/trie/hash.go tests #2049

Merged
merged 14 commits into from
Nov 25, 2021
Merged

chore(trie): lib/trie/hash.go tests #2049

merged 14 commits into from
Nov 25, 2021

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Nov 18, 2021 •
edited
Loading

Changes

  • Near full coverage and deeper tests for lib/trie/hash.go
  • Small production code changes

Tests

go test -race github.com/ChainSafe/gossamer/lib/trie

Issues

Primary Reviewer

@qdm12 qdm12 changed the base branch from development to qdm12-mem-hasher-fix November 18, 2021 11:18
@qdm12 qdm12 mentioned this pull request Nov 18, 2021
Merged
@qdm12 qdm12 force-pushed the qdm12-mem-hasher-fix branch 2 times, most recently from 03a0935 to 6673e86 Compare November 18, 2021 14:51
@codecov
Copy link

codecov bot commented Nov 18, 2021 •
edited
Loading

Codecov Report

Merging #2049 (8274c87) into development (0ad5eb7) will decrease coverage by 0.19%.
The diff coverage is 82.12%.

❗ Current head 8274c87 differs from pull request most recent head e7fbfe5. Consider uploading reports for the commit e7fbfe5 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           development    #2049      +/-   ##
===============================================
- Coverage        60.35%   60.15%   -0.20%     
===============================================
  Files              200      194       -6     
  Lines            27013    26686     -327     
===============================================
- Hits             16303    16053     -250     
+ Misses            8801     8747      -54     
+ Partials          1909     1886      -23
Flag Coverage Δ
unit-tests 60.15% <82.12%> (-0.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/trie/hash.go 69.65% <74.23%> (+5.68%) ⬆️
lib/trie/node.go 79.81% <100.00%> (-0.88%) ⬇️
lib/trie/print.go 83.33% <100.00%> (-0.99%) ⬇️
lib/trie/trie.go 92.22% <100.00%> (ø)
dot/rpc/http.go 56.84% <0.00%> (-26.03%) ⬇️
lib/runtime/offchain/httpset.go 69.81% <0.00%> (-8.57%) ⬇️
lib/babe/epoch.go 60.48% <0.00%> (-2.42%) ⬇️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ad5eb7...e7fbfe5. Read the comment docs.

@qdm12 qdm12 force-pushed the qdm12-mem-hasher-fix branch from 97a2391 to 330195b Compare November 19, 2021 17:07
@qdm12 qdm12 changed the title chore(trie): add more tests to lib/trie chore(trie): lib/trie/hash.go tests Nov 22, 2021
@qdm12 qdm12 marked this pull request as ready for review November 22, 2021 15:50
Base automatically changed from qdm12-mem-hasher-fix to development November 23, 2021 12:05
noot
noot reviewed Nov 24, 2021
View reviewed changes
lib/trie/hash_test.go
Comment on lines -45 to -47
func TestHashBranch(t *testing.T) {
n := &branch{key: generateRandBytes(380), value: generateRandBytes(380)}
n.children[3] = &leaf{key: generateRandBytes(380), value: generateRandBytes(380)}
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add a test for random nodes as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? 🤔
I'm a big fan of deterministic tests.
Unless we need crypto randomness in certain niche cases (i.e.generate uniform distribution etc.)?

noot
noot approved these changes Nov 24, 2021
View reviewed changes
Copy link
Contributor

@noot noot left a comment

Choose a reason for hiding this comment

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

looks good to me! how do you feel about adding more tests using generateRand haha

edwardmack
edwardmack approved these changes Nov 24, 2021
View reviewed changes
Copy link
Contributor

@edwardmack edwardmack left a comment

Choose a reason for hiding this comment

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

Nice work, lgtm!

@qdm12 qdm12 merged commit c89d50c into development Nov 25, 2021
@qdm12 qdm12 deleted the qdm12-trie-tests branch November 25, 2021 15:53
@github-actions
Copy link

github-actions bot commented Dec 3, 2021

🎉 This PR is included in version 0.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

timwu20 pushed a commit to timwu20/gossamer that referenced this pull request Dec 6, 2021
@qdm12 @timwu20
chore(trie): lib/trie/hash.go tests (ChainSafe#2049)
88014c6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noot noot noot approved these changes

@edwardmack edwardmack edwardmack approved these changes

@arijitAD arijitAD Awaiting requested review from arijitAD

@EclesioMeloJunior EclesioMeloJunior Awaiting requested review from EclesioMeloJunior EclesioMeloJunior is a code owner

@jimjbrettj jimjbrettj Awaiting requested review from jimjbrettj jimjbrettj is a code owner

@kishansagathiya kishansagathiya Awaiting requested review from kishansagathiya

@omar391 omar391 Awaiting requested review from omar391

@timwu20 timwu20 Awaiting requested review from timwu20 timwu20 is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@qdm12 @edwardmack @noot