Skip to content

Improve tests in #2224#2314

Merged
t-bast merged 2 commits intozeroconf-scid-aliasfrom
zeroconf-scid-alias-bast
Jun 14, 2022
Merged

Improve tests in #2224#2314
t-bast merged 2 commits intozeroconf-scid-aliasfrom
zeroconf-scid-alias-bast

Conversation

@t-bast
Copy link
Member

@t-bast t-bast commented Jun 14, 2022

This is a PR on #2224 to improve some of the tests.

@t-bast t-bast requested a review from pm47 June 14, 2022 08:54
Comment on lines +216 to +220
Alice.nodeParams
.modify(_.channelConf.expiryDelta).setTo(CltvExpiryDelta(147)),
Alice.nodeParams
.modify(_.relayParams.privateChannelFees.feeProportionalMillionths).setTo(2345)
.modify(_.channelConf.expiryDelta).setTo(CltvExpiryDelta(147)),
Copy link
Member

Choose a reason for hiding this comment

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

Oops

Comment on lines +111 to +115
def waitFor(duration: FiniteDuration): Unit = {
val now = TimestampMilli.now()
eventually(PatienceConfiguration.Timeout(duration * 2), PatienceConfiguration.Interval(50 millis)) {
assert(TimestampMilli.now() - now > duration)
}
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this actually achieves something? The calling thread will still be blocked right?

Copy link
Member Author

@t-bast t-bast Jun 14, 2022

Choose a reason for hiding this comment

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

No it shouldn't, this should be using a spin lock (or something equivalent) and should release the thread after each test (so that other tests can run on that thread between checks), whereas Thread.sleep blocks the thread for the whole duration for everyone.

I haven't actually verified this, but it was the behavior on .NET and the JVM is highly similar, so I expect this to be true.

Copy link
Member Author

@t-bast t-bast Jun 14, 2022

Choose a reason for hiding this comment

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

According to the doc of eventually, it attempts the check then sleeps (I assume with Thread.sleep) for the interval duration, then rinse & repeat. So it's not as good as I hoped, but it's still better than Thread.sleep, because instead of blocking the thread for the whole duration, we block it for smaller chunks, which regularly gives the opportunity for the task scheduler to run something else on this thread.

Copy link
Member

@pm47 pm47 Jun 14, 2022

Choose a reason for hiding this comment

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

Yes, but I think this saves thread time for the additional thread that executes whatever code is inside the eventually. The caller thread still has to wait. Maybe I'm completely wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there is an additional thread that executes the code in the eventually, I believe that most of the time it will just be executed by the caller thread (especially since it doesn't contain a future)?

@t-bast t-bast merged commit 189d101 into zeroconf-scid-alias Jun 14, 2022
@t-bast t-bast deleted the zeroconf-scid-alias-bast branch June 14, 2022 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants