-
Notifications
You must be signed in to change notification settings - Fork 170
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
Add initial test checking progress #94
Conversation
Yeah your understanding is correct -- block 0 is the only block in epoch 0, and block |
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.
Nice work! Just left some minor comments and no blockers from my side.
A question that is less relevant to this PR: I remember some CI environments do not allow the program to use networks (e.g., binding ports). Will this be a problem if we include such tests in CI?
return | ||
} | ||
|
||
<-time.After(2 * time.Second) |
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.
Perhaps consider using the ticker package for polling? something like https://blog.boot.dev/golang/range-over-ticker-in-go-with-immediate-first-tick/
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.
tbh for now I would like to keep this as simple and dependency free as possible, this tests will definitly grow in complexity over time
test/integration_test.go
Outdated
latestResponse, err := tm.NewServiceClient(c).GetLatestBlock(context.Background(), &tm.GetLatestBlockRequest{}) | ||
|
||
if err != nil { | ||
panic("Integration tests failed, due to node failure") |
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.
Would it be helpful to attach the error message in panic?
test/integration_test.go
Outdated
@@ -13,7 +13,9 @@ import ( | |||
appparams "github.com/babylonchain/babylon/app/params" | |||
bbl "github.com/babylonchain/babylon/types" | |||
lightclient "github.com/babylonchain/babylon/x/btclightclient/types" | |||
ec "github.com/babylonchain/babylon/x/epoching/types" |
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.
ec "github.com/babylonchain/babylon/x/epoching/types" | |
epochingtypes "github.com/babylonchain/babylon/x/epoching/types" |
Nothing but would be good to have some consistency with other code.
test/integration_test.go
Outdated
@@ -13,7 +13,9 @@ import ( | |||
appparams "github.com/babylonchain/babylon/app/params" | |||
bbl "github.com/babylonchain/babylon/types" |
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.
I remember @fishermanymc mentioned that bbn will be more widely accepted than bbl.
test/integration_test.go
Outdated
currentEpochResponse, err := epochingClient.CurrentEpoch(context.Background(), &ec.QueryCurrentEpochRequest{}) | ||
|
||
if err != nil { | ||
panic("Querry failed, testnet not running") |
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.
Same, not sure if attaching the error msg for debugging would be helpful
Check epoch progressing.
@SebastianElvis does I understand correctly how epoching works here i.e
block 0 is epoch 0
block 1-10 are epoch 1
block 11 - 20 will be epoch ?