Skip to content

Add tests for warning messages from peers#6805

Closed
positiveblue wants to merge 3 commits into
lightningnetwork:masterfrom
positiveblue:test-warning-messages
Closed

Add tests for warning messages from peers#6805
positiveblue wants to merge 3 commits into
lightningnetwork:masterfrom
positiveblue:test-warning-messages

Conversation

@positiveblue
Copy link
Copy Markdown
Contributor

Change Description

Test that warning messages from our peers do not trigger any action.

Steps to Test

pushd htlcswitch && go test -v -run TestChannelLinkFail && popd

Pull Request Checklist

Testing

  • Your PR passes all CI checks.
  • Tests covering the positive and negative (error paths) are included.

Code Style and Documentation

Comment thread htlcswitch/link_test.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

copy-paste

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

...and typos/grammar issues

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the original test too 🙏

Comment thread htlcswitch/link_test.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't test the reverse. If the channel is failed although this is unexpected, the test should fail.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

totally right, i added the new test case, saw it fail (as it should) and fix it this way but in reality I wasn't testing anything 👎

Fixed in both ways with

if test.shouldFailChannel && timeout {
	require.Fail(t, "expected link errors did not happen "+
		"before timeout")
} else if !test.shouldFailChannel && !timeout {
	require.Fail(t, "unexpected link errors happened")
}

@positiveblue positiveblue force-pushed the test-warning-messages branch 2 times, most recently from faf1295 to df9aec2 Compare August 8, 2022 05:49
Add new execution path based on if we expect the channeil to fail or
not.

Fix format for the lines over 80 characters in the test.
Commit #7b56b67 added support for warning messages from our peers.
This commit adds some testing to ensure that processing those messages
don't fail the channel.
@positiveblue positiveblue force-pushed the test-warning-messages branch from df9aec2 to acd8262 Compare August 8, 2022 06:00
@positiveblue positiveblue requested a review from Crypt-iQ August 8, 2022 17:46
@Roasbeef
Copy link
Copy Markdown
Member

Roasbeef commented Aug 8, 2022

This test confirms everything in the link was ok, but the root issue was that we converted the warning to an error when sending to the link in the first place: #6801 (comment)

@lightninglabs-deploy
Copy link
Copy Markdown
Collaborator

@Crypt-iQ: review reminder
@positiveblue, remember to re-request review from reviewers when ready

Comment thread htlcswitch/link_test.go

// Sent a random warning message.
warning := &lnwire.Warning{
Error: *lnwire.NewError()}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: closing brace should be on newline

@Crypt-iQ
Copy link
Copy Markdown
Collaborator

needs rebase

@positiveblue
Copy link
Copy Markdown
Contributor Author

Close in favor of #6840

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.

5 participants