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

test(core|state|swamp): unify and refactor core/app testing utilities #1551

Merged
merged 3 commits into from
Jan 5, 2023

Conversation

Wondertan
Copy link
Member

@Wondertan Wondertan commented Dec 27, 2022

This PR is one of the cleaning PRs that I am doing. It removes the usage of KVStore. Besides simplifying and unifying tests, it also fixes all the existing and potential "bind already in used" errors from the app and tendermint tandem. I tested this by running all the tests with -parallel 12, which runs in parallel per pkg. With this flag, we can potentially speed up the time it takes to run all the tests on Ci and locally. Going further, we can also consider running all the tests in parallel with only one instance of the app running, rather than an instance per test

Would love this diff to be reddish, but unfortunately, I had to copy a bunch of code from Cosmos SDK. The rationale is in the comments. Also, see this.

@Wondertan Wondertan added area:core_and_app Relationship with Core node and Celestia-App kind:testing Related to unit tests swamp Related to Integration tests labels Dec 27, 2022
@Wondertan Wondertan self-assigned this Dec 27, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1551 (ec5a00a) into main (7e4f27a) will decrease coverage by 0.04%.
The diff coverage is 78.51%.

@@            Coverage Diff             @@
##             main    #1551      +/-   ##
==========================================
- Coverage   55.09%   55.04%   -0.05%     
==========================================
  Files         211      212       +1     
  Lines       12953    13019      +66     
==========================================
+ Hits         7136     7166      +30     
- Misses       5091     5117      +26     
- Partials      726      736      +10     
Impacted Files Coverage Δ
core/grpc.go 65.07% <65.07%> (ø)
core/testing.go 84.05% <87.71%> (+0.47%) ⬆️
nodebuilder/testing.go 100.00% <100.00%> (ø)
nodebuilder/core/constructors.go 0.00% <0.00%> (-100.00%) ⬇️
nodebuilder/core/module.go 76.47% <0.00%> (-17.65%) ⬇️
header/core/listener.go 52.83% <0.00%> (-5.67%) ⬇️
share/eds/byzantine/bad_encoding.go 66.00% <0.00%> (-3.00%) ⬇️

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

distractedm1nd
distractedm1nd previously approved these changes Jan 2, 2023
Copy link
Collaborator

@distractedm1nd distractedm1nd left a comment

Choose a reason for hiding this comment

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

Much cleaner, looks great

Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

Pls rename grpc file to grpc_testing or something to indicate that this functionality is for testing only. Otherwise lgtm and thank you for this :)

@Wondertan Wondertan enabled auto-merge (squash) January 5, 2023 16:14
@Wondertan Wondertan merged commit d9c1ae7 into main Jan 5, 2023
@Wondertan Wondertan deleted the hlib/core/cleanup-test-utils branch January 5, 2023 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core_and_app Relationship with Core node and Celestia-App kind:testing Related to unit tests swamp Related to Integration tests
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants