fundingmanager test: poll or database state instead of using timeouts. #366

halseth · 2017-10-03T20:16:06Z

This commits make the fundingmanager tests poll for database state
for a time, instead of using an explicit sleep before accessing the
DB. This should address some of the flakes encountered on Travis,
where db writes might take longer than usual.

jimpo · 2017-10-06T21:10:11Z

fundingmanager_test.go

I don't think it's possible for err != nil given the checks above.

Agreed 👍

jimpo · 2017-10-06T21:11:00Z

fundingmanager_test.go

I think this would read a bit better as:

if err == ErrChannelNotFound { // Got expected state, return with success. return } else if err != nil { t.Fatalf("unable to get channel state: %v", err) }

does this have the same behavior as the current assertion? or are you suggesting to make them a single if-else?

I think this comment initially was made in assertErrChannelNotFound and magically moved here after I made the change 🚶

jimpo · 2017-10-06T21:13:30Z

fundingmanager_test.go

getChannelOpeningState returns state as markedOpen if there is an error, so we should not return if err == ErrChannelNotFound

good catch!

jimpo · 2017-10-06T21:14:30Z

fundingmanager_test.go

nit: num -> expectedNum

This commits make the fundingmanager tests poll for database state for a time, instead of using an explicit sleep before accessing the DB. This should address some of the flakes encountered on Travis, where db writes might take longer than usual.

halseth · 2017-10-20T10:57:09Z

Thanks for the review @jimpo! Addressed the comments, and should be ready for review/merge 🎊

PTAL.

cc @Roasbeef

cfromknecht

Nice refactor, I like the consistency that this approach adds to our testing framework! A couple comments, but looks solid 🎉 The test cases are definitely simpler to interpret as a result :)

cfromknecht · 2017-10-21T01:04:16Z

fundingmanager_test.go

+		}
+
+		// Try again in 500 ms.
+		time.Sleep(500 * time.Millisecond)


same as above

cfromknecht · 2017-10-21T01:08:27Z

fundingmanager_test.go

does this have the same behavior as the current assertion? or are you suggesting to make them a single if-else?

cfromknecht · 2017-10-21T01:12:56Z

fundingmanager_test.go

+func assertNumPendingChannels(t *testing.T, node *testNode, expectedNum int) {
+	var numPendingChans int
+	for i := 0; i < 10; i++ {
+		pendingChannels, err := node.fundingMgr.cfg.Wallet.Cfg.Database.FetchPendingChannels()


more than 80 chars?

Yep, not sure what's the best way to split such a line, but did my best now :b

awesome, looks good!

cfromknecht · 2017-10-21T01:19:29Z

fundingmanager_test.go

+		}
+
+		// Try again in 500 ms.
+		time.Sleep(500 * time.Millisecond)


same as above

cfromknecht · 2017-10-21T01:20:05Z

fundingmanager_test.go

+		}
+
+		// Sleep, and try again in a bit.
+		time.Sleep(500 * time.Millisecond)


do we need to sleep on the last iteration? another alternative is to sleep at the top of the loop if only if i > 0

cfromknecht · 2017-10-21T01:29:34Z

fundingmanager_test.go

+
+	var state channelOpeningState
+	var err error
+	for i := 0; i < 10; i++ {


Would it be possible to break out the default number of iterations and sleep duration as constants?

…imeouts.

cfromknecht · 2017-10-23T23:00:48Z

Thanks @halseth, LGTM! 🎉😎

Roasbeef

LGTM

jimpo reviewed Oct 6, 2017

View reviewed changes

halseth force-pushed the funding-tests-db-read branch from 8e4a1f8 to 95393dd Compare October 20, 2017 08:25

cfromknecht reviewed Oct 21, 2017

View reviewed changes

fixup! fundingmanager test: poll or database state instead of using t…

eb7ab2b

…imeouts.

Roasbeef approved these changes Oct 25, 2017

View reviewed changes

Roasbeef merged commit 4b883b9 into lightningnetwork:master Oct 25, 2017

halseth deleted the funding-tests-db-read branch July 12, 2018 13:40

fundingmanager test: poll or database state instead of using timeouts. #366

fundingmanager test: poll or database state instead of using timeouts. #366

Uh oh!

Conversation

halseth commented Oct 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

halseth commented Oct 20, 2017

Uh oh!

cfromknecht left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfromknecht commented Oct 23, 2017

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cfromknecht left a comment •

edited

Loading