-
Notifications
You must be signed in to change notification settings - Fork 961
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(swamp/share): cover share module with swamp tests #4036
base: main
Are you sure you want to change the base?
Conversation
06f683d
to
6cd9e3b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4036 +/- ##
==========================================
+ Coverage 44.83% 45.32% +0.48%
==========================================
Files 265 309 +44
Lines 14620 22304 +7684
==========================================
+ Hits 6555 10109 +3554
- Misses 7313 11120 +3807
- Partials 752 1075 +323 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay with me, thank you
"github.com/celestiaorg/celestia-node/state" | ||
) | ||
|
||
func TestShareModule(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is structured a lot like a unit test, which is helpful but not exactly what integration tests(Swamp) are for. Like intergration tests should not test "invalid height" that's a unit test concern. On the other side, this test would benefit from testing GetRow on Q1/Q2 and Q3/Q4 and accessing it from every node type, rather than light only. In simple terms, you want to test things that involve multiple components here(the full node), not the single ifs in one component.
So as actionable I would make sure the tests are working for every node type and move out unit tests out. If moving out units is complicated, I am fine with parking it in the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the structured feedback. Make a lot of sense. Removing unit tests is not a big deal, so I'm fine w removing them within this PR.
self-explanatory