Add integration tests #85
Conversation
KonradStaniec
force-pushed
the
add-integration-tests
branch
5 times, most recently
from
2ad1341
to
26fb05b
Compare
| rpc BestHeader(QueryBestHeaderRequest) returns (QueryBestHeaderResponse) { | ||
| option (google.api.http).get = "/babylon/btclightclient/v1/bestheader"; | ||
| } |
There was a problem hiding this comment.
| rpc BestHeader(QueryBestHeaderRequest) returns (QueryBestHeaderResponse) { | |
| option (google.api.http).get = "/babylon/btclightclient/v1/bestheader"; | |
| } | |
| rpc Tip(QueryTipRequest) returns (QueryTipResponse) { | |
| option (google.api.http).get = "/babylon/btclightclient/v1/tip"; | |
| } |
Nothing against "best header", just curious, why introduce new names for already existing concepts? Do you think "tip" is confusing?
There was a problem hiding this comment.
I find it a bit ambiguous to be honest it can mean tip of the blockchain, or maybe some tip to the miner. It can be interpreted in too many ways to my taste. I find best header having a bit more precise.
But I can change it to tip, to not introduce new terminolgy. We can debate naming of external apis when we will have some kind public testnet, and biger sample of people using those apis.
There was a problem hiding this comment.
Actually I saw that "best block" is a thing in Bitcoin, so it's fine with me.
| // This test serves as a waiting point for testnet to start, it is needed as | ||
| // docker compose is usually started in detached mode in CI, therefore tests | ||
| // are started even before all nodes are up. | ||
| // TODO: investigate starting testnet from golang test file. | ||
| func TestTestnetRuninng(t *testing.T) { |
There was a problem hiding this comment.
According to https://stackoverflow.com/a/31204016 the execution order of tests should not be relied upon. It suggests calling this from init() or TestMain() instead.
There was a problem hiding this comment.
oh I didn't know that.
From what I see one can also provide flag go test -p 1 which will force sequential execution. In general I plan for those tests to be stateful (there will be one testnet running against which there weill be bunch of tx executed) and deterministic, so the execution order of tests will matter, also they do not run with other test due to build tag // +build integration so the won't execute by accident.
So I think I prefer to just add this flag to make target test-babylon-integration and have this smoke test as initial one to just check everything is working.
There was a problem hiding this comment.
If -p 1 means not just sequential but in the order of appearance, great 👍
test/integration_test.go
Outdated
| for i, c := range clients { | ||
| lc := lightclient.NewQueryClient(c) | ||
|
|
||
| res, err := lc.BestHeader(context.Background(), lightclient.NewQueryBestHeaderRequest()) |
There was a problem hiding this comment.
I'm not sure the BestHeader endpoint will always be the best choice to test what the genesis block is, if we assume that an actual local testnet is running. Ostensibly that could include BTC relayers that feed new headers to Babylon, otherwise the testnet will never produce a checkpoint. By the time you run this test, it might already have moved on to a new tip.
The Mainchain method should document what order it returns headers in, but looking at the code you can ask for results to be iterated in reverse in which case it wills tart from the base, and with a limit of 1 you should get the genesis header.
There was a problem hiding this comment.
As mention in earlier comment, I intend for those tests to be stateful and deterministic, so as this will be the first test which will run, then at this point best header should be be genesis header. In later tests, after ingesting few header tip should move by expected amount of headers.
There was a problem hiding this comment.
Sorry that's not what I meant. I meant that according to the PR description you start with make localnet-start; ostensibly that could include a BTC node that starts creating blocks and some relayers, or maybe it could include just relayers who pull data from the BTC testnet, I don't know. A real local testnet would surely not stand still forever.
Admittedly it will probably not move on before this test executes, but just wanted to point out that there is a deterministic endpoint for you to use.
Add:
I will add CI step in next pr as this require some fiddling with ip address so that testsuite can hit testnet containers and this pr is already quite big.
Currently it can be ran locally with: