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

Improve test coverage #350

Merged
merged 23 commits into from
May 11, 2023
Merged

Improve test coverage #350

merged 23 commits into from
May 11, 2023

Conversation

jsign
Copy link
Collaborator

@jsign jsign commented May 1, 2023

This PR's main intention is to improve our test coverage.

As usually happens, looking at coverage profiles, it forces you to see some code in general, and that triggered some overall code improvements like removing duplicated-ish code, adding some extra validation checks, removing unused methods, etc. I’ll explain them in the comments.

Our pre-PR coverage numbers were pretty good. However, 1% of uncovered code in some files could be bad, tree.go in particular; this is natural due to organic code evolution. I ensured all important branches are covered now and left some in-comment TODOs in some places I’d like to dive into later to cover appropriately (proof generation/verification related).

Here’re the pre and post-coverage numbers (i.e: pre% -> post%):

  • Overall coverage: 84.8% → 91.6%
  • Per file:
    • config.go (100.0% → 100%)
    • config_ipa.go (74.1% → 88.9%)
    • conversion.go (80.5% → 80.5%)
    • empty.go (61.5% → 92.3%)
    • encoding.go (93.8% → 95.8%)
    • hashednode.go (62.5% → 87.5%)
    • ipa.go (73.3% → 92.3%)
    • proof_ipa.go (91.9% → 91.9%)
    • proof_json.go (80.7% → 80.7%)
    • tree.go (87.1% → 94.3%)
    • unknown.go (23.1% → 92.3%)

.github/workflows/go.yml Show resolved Hide resolved
config_ipa.go Show resolved Hide resolved
config_ipa_test.go Show resolved Hide resolved
conversion.go Outdated Show resolved Hide resolved
debug_test.go Show resolved Hide resolved
tree_test.go Show resolved Hide resolved
tree_test.go Show resolved Hide resolved
tree_test.go Show resolved Hide resolved
tree_test.go Show resolved Hide resolved
unknown.go Show resolved Hide resolved
@jsign jsign marked this pull request as ready for review May 1, 2023 14:51
@jsign jsign requested a review from gballet May 1, 2023 14:52
Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

Nice, I left a few comments but LGTM overall.

cpu.out Outdated Show resolved Hide resolved
mem.out Outdated Show resolved Hide resolved
go-verkle.test Outdated Show resolved Hide resolved
ipa.go Show resolved Hide resolved
ipa.go Show resolved Hide resolved
tree.go Outdated Show resolved Hide resolved
tree.go Outdated Show resolved Hide resolved
tree.go Outdated Show resolved Hide resolved
tree.go Outdated Show resolved Hide resolved
config_ipa_test.go Show resolved Hide resolved
@jsign
Copy link
Collaborator Author

jsign commented May 2, 2023

Nice, I left a few comments but LGTM overall.

Thanks! I adjusted with some extra commits to many of your comments. I closed all convos except one that I still need to make a decision on; I'll wait for your feedback.

@jsign jsign requested a review from gballet May 2, 2023 13:48
@jsign
Copy link
Collaborator Author

jsign commented May 3, 2023

@gballet, all ready.

jsign added 22 commits May 10, 2023 10:33
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
… other nits

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
… NodeWidth/2

Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Copy link
Member

@gballet gballet left a comment

Choose a reason for hiding this comment

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

LGTM thanks

@gballet gballet merged commit b02bc4f into master May 11, 2023
@jsign jsign deleted the jsign/coverage branch May 11, 2023 12:07
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.

None yet

2 participants