Skip to content

Conversation

@pompon0
Copy link
Contributor

@pompon0 pompon0 commented Oct 10, 2025

@pompon0 pompon0 requested review from masih, sei-will and udpatil October 10, 2025 07:57
@pompon0 pompon0 enabled auto-merge (squash) October 10, 2025 07:58
@codecov
Copy link

codecov bot commented Oct 10, 2025

Codecov Report

❌ Patch coverage is 86.95652% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.56%. Comparing base (2c746c3) to head (8d85be0).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
sei-tendermint/libs/service/service.go 0.00% 3 Missing ⚠️

❌ Your project status has failed because the head coverage (47.23%) is below the target coverage (60.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2458   +/-   ##
=======================================
  Coverage   51.56%   51.56%           
=======================================
  Files        1543     1543           
  Lines      156818   156828   +10     
=======================================
+ Hits        80856    80865    +9     
- Misses      69861    69862    +1     
  Partials     6101     6101           
Flag Coverage Δ
sei-chain 27.51% <0.00%> (+<0.01%) ⬆️
sei-cosmos 51.64% <ø> (+<0.01%) ⬆️
sei-tendermint 58.57% <86.95%> (+<0.01%) ⬆️
sei-wasmd 51.44% <ø> (ø)

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

Files with missing lines Coverage Δ
sei-tendermint/internal/consensus/state.go 70.61% <100.00%> (-0.15%) ⬇️
sei-tendermint/internal/consensus/ticker.go 100.00% <100.00%> (ø)
sei-tendermint/libs/service/service.go 55.71% <0.00%> (-0.81%) ⬇️

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

}
})
if err != nil {
t.Fatal(err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

use testify since the repo depends on it already and other tests use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there are 2 problems with testify:

  • it is weakly typed (I've provided strongly typed wrapper in sei-tendermint/libs/utils/require)
  • require package works only from the test's main goroutine (same as t.Fatal though), assert package is a wrapper on t.Errorf which reports errors lazily (the test actually continues after t.Errorf call, which obfuscates error - extra logs, potential deadlocks, cascading errors).

Hence, I prefer regular error passing (or optionally panics for test helpers, for which error passing gets too verbose). The final t.Fatal call is just for reporting the error back to the testing harness.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK thanks for explaining.

Would be nice to maintain less custom code and converge on using existing popular enough libraries maintained by someone other than us 😅

But both approaches work.

@pompon0 pompon0 merged commit 8210463 into main Oct 10, 2025
104 of 108 checks passed
@pompon0 pompon0 deleted the gprusak-ticker branch October 10, 2025 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants