Skip to content

itest: fix build matrix, WebSocket test timeout and log file upload#5845

Merged
guggero merged 2 commits into
lightningnetwork:masterfrom
guggero:itest-fixes-rest
Oct 25, 2021
Merged

itest: fix build matrix, WebSocket test timeout and log file upload#5845
guggero merged 2 commits into
lightningnetwork:masterfrom
guggero:itest-fixes-rest

Conversation

@guggero
Copy link
Copy Markdown
Collaborator

@guggero guggero commented Oct 8, 2021

Fixes the timeout that sometimes occurred in the REST WebSocket ping/pong test.

Also fixes an issue with the itest failure log file upload that would cause 500 server errors if we tried to upload too many files for one action. We pack all logs before uploading them.

@guggero guggero force-pushed the itest-fixes-rest branch 2 times, most recently from 80323a3 to 59b39e9 Compare October 8, 2021 15:05
@guggero guggero changed the title itest: fix WebSocket test timeout and log file upload itest: fix build matrix, WebSocket test timeout and log file upload Oct 8, 2021
yyforyongyu
yyforyongyu approved these changes Oct 12, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@yyforyongyu yyforyongyu left a comment

Choose a reason for hiding this comment

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

LGTM⚡️
The neutrino backend still times out. Maybe we could get this merged so that other PRs could have the complete build? Pending a release note btw.

Comment thread lntest/itest/lnd_rest_api_test.go Outdated
Copy link
Copy Markdown
Member

@yyforyongyu yyforyongyu Oct 12, 2021

Choose a reason for hiding this comment

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

Maybe we need to check both the errors from WriteMessage and Close (so two requires)?

Comment thread lntest/itest/lnd_rest_api_test.go Outdated
Comment thread lntest/itest/lnd_rest_api_test.go Outdated
Copy link
Copy Markdown
Member

@yyforyongyu yyforyongyu Oct 12, 2021

Choose a reason for hiding this comment

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

👍Probably explains some of the test timeouts? Saw neutrino failed with this message, nice to have it!

Copy link
Copy Markdown
Collaborator Author

@guggero guggero Oct 12, 2021

Choose a reason for hiding this comment

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

Yes, this should explain at least some of the timeouts. Though it looks like we still have a few other flakes, even within the REST API tests...

@Roasbeef
Copy link
Copy Markdown
Member

Roasbeef commented Oct 14, 2021 

    lnd_rest_api_test.go:663: 
        	Error Trace:	lnd_rest_api_test.go:663
        	            				lnd_rest_api_test.go:239
        	Error:      	No invoice msg received in time
        	Test:       	TestLightningNetworkDaemon/tranche03/76-of-87/bitcoind/REST_API/websocket_ping_and_pong_timeout
        	```

New?

@guggero
Copy link
Copy Markdown
Collaborator Author

guggero commented Oct 14, 2021 

    lnd_rest_api_test.go:663: 
        	Error Trace:	lnd_rest_api_test.go:663
        	            				lnd_rest_api_test.go:239
        	Error:      	No invoice msg received in time
        	Test:       	TestLightningNetworkDaemon/tranche03/76-of-87/bitcoind/REST_API/websocket_ping_and_pong_timeout
        	```

New?

I think this is what previously caused the test to block entirely. Now we at least get a failure. Will investigate what causes it.

@Roasbeef
Copy link
Copy Markdown
Member

Roasbeef commented Oct 19, 2021

Needs rebase...assuming we pinpoint that latest failure.

@Roasbeef Roasbeef added this to the v0.14.0 milestone Oct 19, 2021
@guggero
itest: defer close of done channel
423de4d 
The testing framework uses runtime.Goexit() when a test fails which
still allows the defer calls to execute. That's why we should use defer
to close the done channel to allow all goroutines to exit properly.
@guggero
GitHub: fix matrix, package log files before upload
4e224fe 
It turns out we were using the wrong matrix variable in the actual
make command and ran the same itest 6 times with no arguments, all
resulting in running the btcd test.

To avoid uploading too many files in individual requests, we zip them
first before uploading the zip itself.
@guggero
Copy link
Copy Markdown
Collaborator Author

guggero commented Oct 21, 2021

Rebased. Don't see the previous failure anymore. Instead there are new flakes now...

@guggero guggero requested a review from carlaKC October 22, 2021 07:29
carlaKC
carlaKC approved these changes Oct 25, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Code looks good, can't offer much wisdom re new flakes.

@guggero
Copy link
Copy Markdown
Collaborator Author

guggero commented Oct 25, 2021

Going to merge this now to fix the issue of the wrong itests being run. This will make things a bit less green overall but at least doesn't show a distorted picture anymore.

@guggero guggero merged commit ad9c160 into lightningnetwork:master Oct 25, 2021
@guggero guggero deleted the itest-fixes-rest branch October 25, 2021 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yyforyongyu yyforyongyu yyforyongyu approved these changes

@carlaKC carlaKC carlaKC approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v0.14.0

Development

Successfully merging this pull request may close these issues.

4 participants

@guggero @Roasbeef @yyforyongyu @carlaKC