Travis - set context timeouts #1590
Conversation
offerm
changed the title
lnd_test.go
Outdated
| @@ -1025,6 +1028,7 @@ func testUpdateChannelPolicy(net *lntest.NetworkHarness, t *harnessTest) { | |||
| closeChannelAndAssert(ctxt, t, net, net.Alice, chanPoint, false) | |||
| ctxt, _ = context.WithTimeout(ctxb, timeout) | |||
| closeChannelAndAssert(ctxt, t, net, net.Bob, chanPoint2, false) | |||
| ctxt, _ = context.WithTimeout(ctxb, timeout) | |||
There was a problem hiding this comment.
👍 Good catch! The ctxt, _ = context.WithTimeout below can be removed, as it is obviously misplaced.
There was a problem hiding this comment.
Removed. Thanks for noticing
htlcswitch/switch_test.go
Outdated
| // without this the code handling the logTicker and fwdEventicker | ||
| // may not be called (randomally) during coverage tests and there will | ||
| // be a drop in coverage stats (build fail) | ||
| time.Sleep(16 * time.Second) |
There was a problem hiding this comment.
We don't really want to introduce more sleeps in the tests...
There was a problem hiding this comment.
@halseth Please let me know what you want to do to as an alternative.
As ugly as this sleep is, without it we can't be sure that these two tickers actually ticks. If the tests run fast (stronger, not loaded machine) the tickers will not tick and the relevant code will not execute. This will cause a failure in coveralls step.
If you want to have a consistent build process, we better have this sleep (or look for something more complicated to ensure the ticker execution).
Let me know what you suggest.
There was a problem hiding this comment.
Take a look at how other tickers are being mocked:
Line 921 in e0baa49
There was a problem hiding this comment.
Well, I don't think that we want to mock or test these tickers here. The intention is that coveralls produces consistent results.
Mocking the ticker will prevent that code from running and reduce coverage so what will we gain.
I spend a lot of time on the analysis of the reasons for Travis and coveralls failures. I'm not a maintainer of this project but if I were, I would first make sure there is a stable test/build environment and only after that work on improvements.
The current situation where a PR may fail on different reasons which have nothing to do with the change done in that PR is not good.
I don't see what is wrong with these 16 seconds sleep.
There was a problem hiding this comment.
@halseth BTW, see last checks of this simple PR. 2 failures which are unrelated to the change. At least coversall.io is green.
There was a problem hiding this comment.
If you mock the ticker, you can make it tick immediately, and won't have to wait the 16 seconds on it to happen?
There was a problem hiding this comment.
@halseth please note that the ticker related change is now on its own PR.
|
The changes to Also note that a recent PR of mine alters how timeouts are set during integration tests: #1621 |
|
Yes, I can make it two separate PRs. Will be done next week when I'm back from my vacation. |
some RPC calls are reusing the context of priors calls without resetting the timeout to 15 seconds. In a loaded system this may lead to a random error since the 15 seconds may not cover all RPC calls. removed one misplaced context setting
offerm
changed the title
|
Blocked by #1621 |
|
@halseth not that I understand why #1621 should block this one. My original PR #1493 was labeled as P2 by @Roasbeef. I spend a lot of time analyzing each and every failure I had. I found several issues. Some of them impact only the build process and others also relevant to production execution. You have asked me to break the PR into small PRs. I spend time also on doing this just to find that most of these PRs are not even getting attention. Looks like you are not really open to external contribution. |
|
@offerm We appreciate your contributions, really! Right now we are preparing the v0.5 release, that's why some PRs (you are not alone) might not get all the attention they should have, as we have a very limited number of people and hours. Will get to it eventually, though! |
some RPC calls are reusing the context of priors calls without resetting the timeout to 15 seconds. In a loaded system, this may lead to a random error since the 15 seconds may not cover all RPC calls.