[code-health]: reduce duplication in brontide test. #8631
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent changes focus on refining the testing framework for peer-related functionalities in the project, emphasizing readability, maintainability, and test environment structuring. These updates optimize test peer creation, channel management, and overall test organization for improved efficiency and clarity. Changes
Poem by CodeRabbit 🐇
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- peer/brontide_test.go (14 hunks)
- peer/test_utils.go (12 hunks)
Additional comments not posted (13)
peer/test_utils.go (4)
53-64: IntroducetestPeerChanEnvfor structured test environment setup.This new structure organizes the environment setup for testing peer channels, enhancing readability and maintainability.
50-93: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [66-333]
Refactor
createTestPeerintocreateTestPeerWithChannel.The refactoring significantly improves the test setup by separating concerns between peer creation and channel setup. However, consider further modularizing the function to reduce its length and complexity.
- func createTestPeerWithChannel(t *testing.T, updateChan func(a, b *channeldb.OpenChannel)) (*testPeerChanEnv, error) { + // Suggested refactor into smaller, more focused functions.
534-728: IntroducetestPeerParamsfor managing test parameters.The introduction of
testPeerParamsstructure is a positive change, making the test setup more organized and easier to manage.
731-805: IntroducestartPeerfor starting a peer with optional checks.The addition of
startPeerfunction, which includes optional checks, is a good practice for ensuring the peer starts as expected in tests. This enhances the robustness of the test suite.peer/brontide_test.go (9)
38-43: Consider using a helper function for common test setup steps.To improve code maintainability and reduce duplication, consider extracting the common setup steps into a helper function. This can include creating the test harness, extracting peers, and channels from the harness.
78-86: Duplicate setup code detected.The setup code in this test function is similar to the setup in previous test functions. Consider refactoring to use a shared setup function to reduce code duplication and improve maintainability.
178-186: Repeated test setup code.The test setup code is repeated across multiple test functions. Refactoring this into a shared setup function could enhance code readability and maintainability.
297-305: Repeated setup code in test functions.The setup code is repeated in several test functions. Creating a shared setup function could help in reducing duplication and improving the maintainability of the test code.
479-487: Consider refactoring repeated setup code.The setup code is repeated across multiple test functions. Refactoring this into a shared setup function could improve code maintainability and readability.
802-812: Use of a specific update function in test setup.The use of
test.updatein the test setup is a good practice for customizing the test environment. Ensure that this pattern is consistently applied across all tests for better flexibility and maintainability.
965-972: Setting feature bits for testing.The approach of setting feature bits directly on the peer's configuration for testing is effective. Ensure that the test cases cover all relevant combinations of feature bits to thoroughly test the behavior under different configurations.
1279-1285: Marking channel as borked for testing.Marking the channel as borked to simplify the test setup is a practical approach. However, ensure that this does not mask potential issues that could arise in scenarios where the channel is not marked as borked.
1325-1330: Testing removal of pending channels.The test for removing pending channels is important for ensuring the correct management of channel states. Consider adding more edge cases, such as attempting to remove a channel that was never added, to ensure robustness.
peer/test_utils.go
Outdated
| @@ -726,3 +727,79 @@ func createTestPeer(t *testing.T) *testPeerParams { | |||
| chanStatusMgr: chanStatusMgr, | |||
| } | |||
| } | |||
|
|
|||
| func startPeer(t *testing.T, mockConn *mockMessageConn, | |||
| peer *Brontide, extraChecks ...func() error) { | |||
There was a problem hiding this comment.
Please don't do this extraChecks thing. It doesn't operate on any values that are only accessible from the inner scope and makes the API much worse.
There was a problem hiding this comment.
What are your suggestions on extending this function. See how it is used here:
Lines 1465 to 1495 in 15c3107
There was a problem hiding this comment.
just run that function after startPeer there's no reason for it to be inside of the startPeer scope.
There was a problem hiding this comment.
That check has to happen while the peer is starting up, we are testing for sending peer their data on reconnection. It happens while starting up the peer.
There was a problem hiding this comment.
The function takes no arguments so its entire closure environment is captured prior to the run. If you are depending on the right ordering of state mutations of values defined outside the environment then this design is bad as it is thoroughly unintuitive and offers no explanation to the caller what should or should not be supplied in this list. Find another way.
There was a problem hiding this comment.
If I make the closure accept mockconn as an argument, would it make this okay? or maybe buffer the channel but I will see how that would go.
There was a problem hiding this comment.
I removed it, I will just add a bool value to it and write the test in the goroutine itself, when I need to extend,
peer/test_utils.go
Outdated
| @@ -643,3 +529,200 @@ func (m *mockMessageConn) LocalAddr() net.Addr { | |||
| func (m *mockMessageConn) Close() error { | |||
| return nil | |||
| } | |||
|
|
|||
| type testPeerParams struct { | |||
There was a problem hiding this comment.
This is a friendly reminder that "params" is not a good struct name. Ask yourself why you are grouping it into a struct. How does this grouping of values function?
There was a problem hiding this comment.
How about testPeerEnv?
There was a problem hiding this comment.
Could you offer an explanation as to why this particular set of struct properties was selected? I suspect the answer to this naming question lies in that explanation.
There was a problem hiding this comment.
They are values used in the function that callers of the startPeer function would like to have access to for tests
There was a problem hiding this comment.
All tests? What kinds of tests? What are you testing? I encourage you to think a bit deeper than the obvious representational description.
There was a problem hiding this comment.
If you want to suggest a more descriptive name, you can go ahead, if not testPeerEnv seems good to me.
There was a problem hiding this comment.
call it peerTestContext
There was a problem hiding this comment.
I think testPeerEnv and peerTestContext are the same?
There was a problem hiding this comment.
We already have examples in other parts of the codebase like
https://github.com/lightningnetwork/lnd/blob/master/htlcswitch/link_isolated_test.go#L13-L20
so my suggestion is to keep things consistent.
Chinwendu20
changed the title
Chinwendu20
force-pushed
the
brontidetest
branch
from
7930403
to
eaba9c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- peer/brontide_test.go (14 hunks)
- peer/test_utils.go (12 hunks)
Files skipped from review as they are similar to previous changes (1)
- peer/test_utils.go
Additional comments not posted (11)
peer/brontide_test.go (11)
38-41: Consider consolidating the setup code for creating test peers with channels into a helper function to reduce duplication across tests.
78-81: Ensure that the test setup code is not unnecessarily duplicated across test functions. If similar setup is used in multiple tests, consider using a setup helper function.
178-181: Repeated test setup code detected. Creating a shared setup function could improve maintainability and readability.
297-300: To avoid code duplication in test setups, consider implementing a shared setup function that can be reused across tests.
479-482: The pattern of duplicating test setup code is observed again. Utilizing a shared setup function could enhance code reuse and test structure.
802-812: The test setup code for creating a test peer with a channel is repeated. Consider refactoring this into a shared setup function to improve code reuse and readability.
965-972: The setup code for creating a test peer and setting feature bits is repeated across tests. Refactoring this into a shared setup function could streamline the test code.
1007-1011: Repeated setup code for creating a test peer and setting up custom message handling is observed. Consider refactoring into a shared setup function to enhance code organization.
1052-1057: The setup code for creating a test peer with a channel is duplicated. Implementing a shared setup function could improve code maintainability.
1279-1285: The pattern of duplicating setup code for creating a test peer with a channel is noted again. A shared setup function could simplify the test code structure.
1325-1330: Repeated setup code for creating a test peer with a channel is observed. Refactoring this into a shared setup function could enhance code reuse and test clarity.
Chinwendu20
force-pushed
the
brontidetest
branch
from
eaba9c8
to
957868f
Compare
peer/test_utils.go
Outdated
| // createTestPeerWithChannel creates a channel between two nodes, and returns a | ||
| // peer for one of the nodes, together with the channel seen from both nodes. | ||
| // It takes an updateChan function which can be used to modify the default | ||
| // values on the channel states for each peer. | ||
| func createTestPeerWithChannel(t *testing.T, notifier chainntnfs.ChainNotifier, | ||
| publTx chan *wire.MsgTx, updateChan func(a, b *channeldb.OpenChannel), | ||
| mockSwitch *mockMessageSwitch) ( | ||
| *Brontide, *lnwallet.LightningChannel, error) { | ||
| *testPeerChanEnv, error) { |
There was a problem hiding this comment.
This doesn't need to return a pointer to the testPeerChanEnv
There was a problem hiding this comment.
So that one can easily return nil? I also read that working with pointers is more memory efficient instead of copying struct into different memory addresses. It is also in line with what I have seen in the codebase
There was a problem hiding this comment.
In this case you can leave it. Discussions of efficiency are highly nuanced, and what you read isn't wrong, but it isn't right either. Also we are moving away from excessive pointer usage throughout the codebase. The reason to move away from pointer usage has to do with making the codebase less fragile over its lifetime when many many people will touch it. Robustness > efficiency 99% of the time.
peer/test_utils.go
Outdated
| @@ -643,3 +529,200 @@ func (m *mockMessageConn) LocalAddr() net.Addr { | |||
| func (m *mockMessageConn) Close() error { | |||
| return nil | |||
| } | |||
|
|
|||
| type peerTestCtx struct { | |||
There was a problem hiding this comment.
how does this differ from the earlier specified testPeerChanEnv?
I actually think this is a pretty instructive example 🙂.
In the past you may have heard me say that naming things properly is important. This is what I mean when I say that bad naming can get out of control. If you don't have concrete concepts that bind things together, then you can end up in situations like this where we have a whole bunch of things that are almost the same with names that are vague and this makes it hard to understand when reading the code and even harder to remember which of these structs you need when writing new code.
Here we have two structs that seem to share a similar role and they have different permutations of the same vague name. How do I know when to use one instead of the other. Or do I give up and create a third
Either we need to consolidate these two ideas into a single one, or we need to clearly specify their difference and denote that in the naming and godoc so that people who touch this code next know which type to use in which situation. Naming may feel trivial but it reveals an imprecision in the ideas themselves.
So answer me this: why can't we use this instead of testPeerChanEnv?
There was a problem hiding this comment.
On a second look, I think maybe it can be one struct. I have consolidated them.
There was a problem hiding this comment.
It looks like there is a commit in the middle where you have two copies of the same struct defined. You'll need to consolidate them in that fourth commit, rather than the last one, if you want the CI to accept it.
Chinwendu20
force-pushed
the
brontidetest
branch
2 times, most recently
from
e682b30
to
8705c38
Compare
ProofOfKeags
changed the title
In this function `createTestPeer` is changed to `createTestPeerWithChannel`. This is useful in coming commits where we decouple the process of creating test peer from the function. Signed-off-by: Ononiwu Maureen <[email protected]>
The `createTestPeerWithChannel` function is made to return this newly created struct and error. This is useful because upcoming commits would require us returning more objects from the function. Signed-off-by: Ononiwu Maureen <[email protected]>
test_utils Signed-off-by: Ononiwu Maureen <[email protected]>
Signed-off-by: Ononiwu Maureen <[email protected]>
Signed-off-by: Ononiwu Maureen <[email protected]>
|
@Chinwendu20, remember to re-request review from reviewers when ready |
Change Description
This pull request is an off shoot of the peer backup PR (currently halted): #8490
It removes the duplicity in creating a peer instance and starting a peer in brontide tests.
It does not require a change log.